fix: Do not pass undeclared feature view columns to ODFV UDFs - #6527
Conversation
| # stay. substrait filters on its own, so leave it alone. | ||
| declared_source_names = { | ||
| projection.name | ||
| for projection in odfv.source_feature_view_projections.values() |
There was a problem hiding this comment.
FeatureViewProjection can carry a name_alias or version tag, but these comparisons and column names use .name. For an ODFV sourced from an aliased projection, won’t the alias-qualified input be classified as undeclared while alias__feature also escapes the second filter? Should these use projection.name_to_use() instead?
There was a problem hiding this comment.
Thanks @Sanjays2402 — fair question, so I verified it against this branch (checked out this PR's head) rather than guess.
With an ODFV sourced from an aliased projection:
aliased = driver_stats.with_name("aliased_stats")
# projection.name = 'driver_stats', name_to_use() = 'aliased_stats'the columns feeding the transform come through as ['driver_stats__conv_rate', 'conv_rate'] — .name-qualified, not alias-qualified. That's consistent with how source refs are built throughout on_demand_feature_view.py (f"{source_fv_projection.name}__{feature.name}", e.g. the loops around lines 987/1055/1151/1261). So .name matches the real column names here; switching to name_to_use() would build aliased_stats__conv_rate, which isn't present, and would drop the declared feature.
On version_tag: it's only set via from_proto (there's no user-facing setter), and the retrieval path keys source columns by .name regardless — so even a versioned projection's column is still driver_stats__conv_rate.
I'll add a test with an aliased source to lock this behavior in. If you know a path (offline retrieval?) where the input comes through alias-qualified, point me at it and I'll handle that too — I exercised online retrieval here.
73a4b10 to
883dc2d
Compare
883dc2d to
797ed12
Compare
ntkathole
changed the title
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6527 +/- ##
==========================================
+ Coverage 46.37% 46.40% +0.02%
==========================================
Files 414 414
Lines 50089 50109 +20
Branches 7159 7167 +8
==========================================
+ Hits 23231 23254 +23
+ Misses 25231 25228 -3
Partials 1627 1627
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
An OnDemandFeatureView's UDF was receiving every feature column in the online response, including features from feature views that the ODFV did not list in its sources. That allowed a UDF to silently depend on an undeclared source. This filters the UDF input down to the ODFV's declared sources before the transform runs, so columns from undeclared feature views are hidden. Join keys, request data and the declared features are kept. substrait already restricts its inputs through its query plan, so it is left as is. Fixes feast-dev#6158. Signed-off-by: Vedant Agarwal <vedantagwl10@gmail.com>
Add a regression test that sources a pandas ODFV from an aliased feature view (with_name) and asserts the declared feature still reaches the UDF while an unrelated feature view stays hidden. The isolation filter keys columns by projection.name, which is what the retrieval path emits regardless of the alias; the test fails if that is switched to name_to_use(). Signed-off-by: Vedant Agarwal <vedantagwl10@gmail.com>
797ed12 to
8bb6764
Compare
…dev#6527) * fix: do not pass undeclared feature view columns to ODFV UDFs An OnDemandFeatureView's UDF was receiving every feature column in the online response, including features from feature views that the ODFV did not list in its sources. That allowed a UDF to silently depend on an undeclared source. This filters the UDF input down to the ODFV's declared sources before the transform runs, so columns from undeclared feature views are hidden. Join keys, request data and the declared features are kept. substrait already restricts its inputs through its query plan, so it is left as is. Fixes feast-dev#6158. Signed-off-by: Vedant Agarwal <vedantagwl10@gmail.com> * test: cover ODFV source isolation for an aliased source Add a regression test that sources a pandas ODFV from an aliased feature view (with_name) and asserts the declared feature still reaches the UDF while an unrelated feature view stays hidden. The isolation filter keys columns by projection.name, which is what the retrieval path emits regardless of the alias; the test fails if that is switched to name_to_use(). Signed-off-by: Vedant Agarwal <vedantagwl10@gmail.com> --------- Signed-off-by: Vedant Agarwal <vedantagwl10@gmail.com>
4 participants
What
An OnDemandFeatureView's UDF (pandas and python modes) was receiving every feature column in the online response, including features from feature views that the ODFV did not list in its sources. This allowed a UDF to silently depend on an undeclared source.
This filters the UDF input down to the ODFV's declared sources before the transform runs. Join keys, request-source fields and the declared features are kept; only columns from undeclared feature views are removed. substrait already restricts its inputs through its query plan, so it is left as is.
Fixes #6158.
Tests
Three tests were added in
test_on_demand_pandas_transformation.py:Each of these fails on the current code and passes with this change.
Functional test results:
ruffandmypyare clean.Scope
This covers the online path (
get_online_features), which is what the issue reports. The offline path (get_historical_features) has the same shape and can be addressed as a follow-up.