fix: preserve millisecond precision for UNIX_TIMESTAMP sort keys#369
Open
piket wants to merge 7 commits into
Open
fix: preserve millisecond precision for UNIX_TIMESTAMP sort keys#369piket wants to merge 7 commits into
piket wants to merge 7 commits into
Conversation
…C-22316) SortedFeatureView rows whose UNIX_TIMESTAMP sort keys differed by less than one second were being assigned the same sort key score in Redis/Valkey (wrong ordering) and the same Cassandra clustering key (row overwrites). Root cause: _python_datetime_to_int_timestamp() truncated all UNIX_TIMESTAMP values to integer seconds before storing in unix_timestamp_val. The * 1000 multiplications in zset_score() attempted ms precision but had no effect. Fix: - Add _python_datetime_to_int_ms_timestamp() for millisecond conversion. - In _convert_arrow_fv_to_proto(), detect UNIX_TIMESTAMP sort key columns and use the ms function for those columns only; all other UNIX_TIMESTAMP columns remain at second precision (backward compatible). - Remove the * 1000 from zset_score() in Redis, Valkey, and Cassandra — unix_timestamp_val is now already ms for sort key columns. - Add a threshold discriminator (val > 1e11) in feast_value_type_to_python_type() so sort key ms values are correctly read back as datetimes without affecting the seconds interpretation for regular feature columns. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zabarn
force-pushed
the
fix/EAPC-22316-sorted-fv-timestamp-precision
branch
2 times, most recently
from
721115e to
2e760ed
Compare
…s (EAPC-22316) After the Python SDK was updated to store sort key unix_timestamp_val as milliseconds, the Go serving layer needed matching changes to avoid treating ms values as epoch-seconds (which would produce year ~57000 timestamps). Production changes: - typeconversion.go: add unixTsToTime() helper with a >1e11 threshold to distinguish ms from s; use it for all UnixTimestampVal conversions - redis_read_range_utils.go: use UnixMilli() for ZRANGEBYSCORE bounds to match the ms ZADD scores written by the Python materializer Test/infra changes: - go_integration_test_utils.go: support FEAST_BIN env var for local venv feast; return UnixMilli() from ReadParquetDynamically for timestamp columns - valkey_integration_test.go: fix ms/s conversions in getValueType and EventTimestamps assertions - scylladb_integration_test.go: update equals filter to ms precision (1744769171919) - http_integration_test.go: update equals filter to ms precision (1744769171919) - redisonlinestore_test.go, redis_read_range_utils_test.go: update expected ZADD scores and range bounds to ms scale Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…keys (EAPC-22316) InterfaceToProtoValue's `time.Time` case truncated to whole seconds via .Unix(), even though sort key UNIX_TIMESTAMP columns are always written at millisecond precision (per the earlier fix in this branch). This is the gocql-specific path: only SortedFeatureView sort key columns are stored as native Cassandra/Scylla `timestamp` columns (regular features are opaque blobs read via UnmarshalStoredProto), so a raw time.Time only ever reaches this function for ms-precision sort keys. Reproduced against a real Cassandra instance: after `feast materialize`, cqlsh showed three rows with fully distinct millisecond clustering values, but /get-online-features-range collapsed two of them (18ms apart) to an identical second-precision string. Fix: - InterfaceToProtoValue: time.Time/[]time.Time cases now use UnixMilli() instead of Unix(), matching the >1e11 ms/seconds threshold already used by the decode side (unixTsToTime). - Added GetTimestampMillis helper; used it for the EventTimestamps metadata field in grpc_server.go, which had the same truncation bug. - Added regression tests: typeconversion_test.go (round-trip + distinctness of sub-second values), serving_test.go (range value-conversion loop with two sort-key timestamps 18ms apart), and a new ScyllaDB integration test + fixture (regression_repo.py / sub_second_data.parquet) covering the same scenario against a real backend. Verified end-to-end: rebuilt the Go server from this commit and re-ran the exact repro (real Cassandra + feast materialize) that caught the bug - /get-online-features-range now returns three distinct millisecond-precision values instead of two collapsed ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…316)
The existing sub-second regression fixture used "event_timestamp" as the
sort key name - the same name used everywhere else in this test suite.
Add a second fixture where the sort key is a distinctly-named feature
("viewed_at"), separate from the source's own "event_timestamp" watermark
field, mirroring the real customer schema (personalization_pdp_features)
that originally surfaced this bug.
Guards against the fix being accidentally tied to the "event_timestamp"
column name rather than working for any UNIX_TIMESTAMP column declared as
a sort key.
Verified against a real Cassandra instance: materialized both fixtures,
confirmed via cqlsh that the new table's clustering column is literally
named "viewed_at" with full millisecond precision, while the independent
"event_ts" watermark column remains at second precision as expected. Both
new Go integration tests pass with the exact expected millisecond values.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…22316) _python_datetime_to_int_ms_timestamp's fallback for non-datetime values (`else: int_timestamps.append(int(value))`) was copied verbatim from the sibling seconds-precision function and silently assumed any raw integer was already milliseconds. A raw (non-datetime) value reaching this branch means the sort key column arrived as a plain integer rather than a typed timestamp - e.g. a Spark LongType column instead of TimestampType, which happens if the source Avro schema's field lacks a timestamp-millis/ timestamp-micros logicalType. There's no way to know the unit of a bare integer without a heuristic, and this code used none. Fix: apply the same magnitude threshold already used everywhere else in this codebase (MS_TIMESTAMP_THRESHOLD = 1e11, matching unixTsToTime in Go and feast_value_type_to_python_type here) to decide whether a raw integer is already milliseconds or is seconds needing conversion, instead of assuming. Also logs a warning, since hitting this branch at all for a sort key column is itself a signal of an upstream typing issue worth surfacing rather than silently masking. Extracted the threshold into a shared MS_TIMESTAMP_THRESHOLD constant used by both the read-side (feast_value_type_to_python_type) and write-side (_python_datetime_to_int_ms_timestamp) disambiguation, replacing the previously duplicated inline 1e11 literals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two related observability additions, motivated by recurring pain points
while diagnosing EAPC-22316: "which build is actually running?" and "is
the write side actually processing what I think it's processing?"
Always-on version banner:
- Fixed a pre-existing bug in version.get_version(): this repo publishes
its SDK distribution as "eg-feast" (see setup.py), not "feast", so the
existing importlib.metadata.version("feast") lookup has been silently
returning "unknown" in every real deployment of this fork. Now checks
both names. This also fixes three other existing callers that were
affected (utils.py's USER_AGENT, transformation_server.py, and
lambda_engine.py's Lambda tags) - no changes needed there, they just
get a working version now.
- Added get_installed_version(package_name), an opportunistic lookup for
a wrapper package (e.g. the "feature-store-materialization" job image)
that returns "not installed" gracefully if absent.
- Both batch materialization entry points (_print_materialization_log,
covering materialize() and materialize_incremental()) and the
streaming entry point (SparkKafkaProcessor.ingest_stream_feature_view)
now log "feast=<version> feature-store-materialization=<version>" on
startup, unconditionally - no config needed to see it.
Opt-in materialized-row logging:
- New RepoConfig fields: log_materialized_rows (default False) and
log_materialized_rows_limit (default 100). Off by default since
enabling it logs raw entity/feature data, which may include PII.
- _convert_arrow_to_proto (the single choke point all three
materialization write paths converge on - local batch engine, Spark
batch engine, and Spark Structured Streaming) gains an optional
log_row_limit parameter; when set, logs a summary line plus up to
`limit` per-row lines (entity key, feature values, timestamps) via a
new _log_materialized_rows helper.
- Wired through all three call sites: local/nodes.py's LocalOutputNode,
spark/utils.py's map_in_arrow and map_in_pandas, and
passthrough_provider.py's process().
- Fixed an existing test (test_nodes.py's create_context) that used a
bare, unconfigured MagicMock for repo_config - truthy by default, so
it was silently exercising the new logging path with mock objects as
the row limit and crashing. Now explicitly sets log_materialized_rows
= False, matching the real default.
Tests: 6 new tests for get_version()/get_installed_version() fallback
behavior (test_version.py), 4 new tests for the row-logging hook
covering the off/on/capped cases and side-effect-free return value
(test_convert_arrow_logging.py). Verified no regressions across
test_type_map.py, test_feature_views.py, repo_config, online_store, and
compute_engines test suites (198 passed, 5 pre-existing skips).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
SortedFeatureView rows whose UNIX_TIMESTAMP sort keys differed by less than one second were being assigned the same sort key score in Redis/Valkey (wrong ordering) and the same Cassandra clustering key (row overwrites).
Root cause: _python_datetime_to_int_timestamp() truncated all UNIX_TIMESTAMP values to integer seconds before storing in unix_timestamp_val. The * 1000 multiplications in zset_score() attempted ms precision but had no effect.
Fix:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Misc