feat: Support typed default values for missing feature values - #6716
Draft
addenergyx wants to merge 12 commits into
Draft
feat: Support typed default values for missing feature values#6716addenergyx wants to merge 12 commits into
addenergyx wants to merge 12 commits into
Conversation
Signed-off-by: David <david-adeniji@hotmail.co.uk>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6716 +/- ##
==========================================
+ Coverage 46.75% 46.94% +0.19%
==========================================
Files 414 414
Lines 50348 50518 +170
Branches 7208 7242 +34
==========================================
+ Hits 23539 23716 +177
+ Misses 25157 25136 -21
- Partials 1652 1666 +14
*This pull request uses carry forward flags. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
…projections Signed-off-by: David <david-adeniji@hotmail.co.uk>
Signed-off-by: David <david-adeniji@hotmail.co.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Implements #6715. A field can declare a typed default that Feast substitutes when the feature is missing or null, so every consumer stops imputing it separately and training and serving agree.
FeatureSpecV2gainsfeast.types.Value default_value = 8. Message presence separates "no default" from a configured0,0.0,falseor"", soHasFieldis used rather than truthiness.Applied in Python online retrieval, Python historical retrieval, and Go serving — in both Python paths before ODFVs run, so a transformation sees the same source value online and offline. Scalar defaults are pushed into the point-in-time query as
COALESCE, so warehouse exports stay server-side instead of dragging the result through pandas; anything not expressible as a portable SQL literal falls back to filling in Python. With no defaults declared the generated SQL is byte-identical to before.Note on string literals: no escaping is portable across the dialects that share this template. BigQuery rejects the SQL-standard
''form, parsing'O''Brien'as two adjacent literals, while backslash escapes are literal in Trino. A string default containing a quote, backslash or newline is therefore filled in Python rather than pushed down. Plain strings push down normally.Statuses are unchanged: a defaulted feature still reports
NOT_FOUND. #6270 added a Prometheus counter read from that same status vector, so marking defaultsPRESENTwould blind it exactly when an upstream materialization job breaks. Values outside TTL keep their real value andOUTSIDE_MAX_AGE.Validation rejects a float an integer dtype would truncate (
Int64with1.5) and values the type cannot represent, but does not compare round-trips by value — that would reject0.1onFloat32. Mutable defaults are deep-copied. Two views exposing the same feature name with different defaults raise underfull_feature_names=Falserather than silently keeping one.Known gaps
to_arrow/to_dfand short-circuit when there are no ODFVs — exactly the defaults-only case. Ibis and Trinopersistwrite server-side without it. The deeper fix is a non-overridable base hook so a native export path must declare itself faithful; that touches ~15 job classes and belongs in its own PR.FeatureStore.get_historical_features.PRESENTregardless of value, which looks like a separate pre-existing bug.ValueType, not the field schema.Separately,
IbisRetrievalJob.persist()never consultson_demand_feature_views, so ibis saved datasets appear to drop ODFV columns today, independent of this change. Happy to raise that as its own issue.Which issue(s) this PR fixes:
Fixes #6715
Checks
git commit -s)Testing Strategy
Unit tests cover the schema (round-trip including zero-like and array defaults, old registries without field 8, rejection rules), Python retrieval (missing row, unset value and explicit
null_val; statuses preserved; mixed batches;full_feature_names; mismatched Arrow column types; the ODFV ordering guarantee; SQL literal escaping), and the Go path.Two caveats on how much the green tick is worth:
BigQuery was verified end-to-end against a real dataset — present values untouched, a null on an existing row and a missing entity row both defaulted, plain string defaults pushed down, and the no-defaults baseline still returning nulls. That run is what caught the string-escaping problem above; generated-SQL assertions alone would not have.
Snowflake, Redshift, Athena, Trino, Spark and Clickhouse remain unexecuted — I have no instance of them. Their pushdown is verified as generated SQL only.
COALESCEand the remaining literal forms are standard across them, and the no-defaults output is unchanged so existing queries cannot regress, but integration tests are the first real check for the defaulted path.Regressions were checked differentially —
sdk/python/tests/unitrun on this branch and onmasterwith the same command, identical failure sets. My environment has pre-existing failures from missing optional extras, so that is not a green suite.go build,go vetandgo test ./go/internal/feast/onlineserving/...pass.Misc
The regenerated
_pb2files usegrpcio-tools==1.60.1/mypy-protobuf==3.3.0to match the tree, which is all gencode 4.25.1 with noruntime_versionvalidation. A currentgrpcio-toolsemits a runtime check requiring protobuf >= 7, which would break CI (protobuf==6.33.6) and users on the declared>=4.24.0floor. Only incidental diff is the header moving to4.25.0. Happy to redo this if there is a preferred pinned toolchain.Release note: