fix: Preserve millisecond precision for UNIX_TIMESTAMP sort keys#369
Open
piket wants to merge 10 commits into
Open
fix: Preserve millisecond precision for UNIX_TIMESTAMP sort keys#369piket wants to merge 10 commits into
piket wants to merge 10 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
June 30, 2026 19:00
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>
…ging
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.
Always-on row-count logging:
- _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) now logs a
"[materialize] feature_view=<name> rows=<n>" line after every batch,
unconditionally. No raw entity/feature data is logged.
Tests: 6 new tests for get_version()/get_installed_version() fallback
behavior (test_version.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>
piket
force-pushed
the
fix/EAPC-22316-sorted-fv-timestamp-precision
branch
from
July 17, 2026 17:06
41275a9 to
7bdaa69
Compare
…s sort keys (EAPC-22316) grpc_server.go used GetTimestampMillis() when building EventTimestamps in the GetOnlineFeaturesRange response. EventTimestamps come from the _ts:<fv> hash field, which stores a timestamppb.Timestamp (seconds/nanos) written by the Python online-write path — they are NOT sort-key scores. Returning them as milliseconds produced values 1000x too large vs what the Valkey integration tests expect. Changed to GetTimestampSeconds(), restoring the original behaviour. HTTP fixture updates: event_timestamp is both the sort key and a schema field in all_dtypes_sorted. After the EAPC-22316 materialisation fix it is stored as milliseconds, so Go's ValueTypeToGoTypeTimestampAsString now formats it with sub-second precision (e.g. "2025-04-16 02:06:10.919Z" instead of "2025-04-16 02:06:10Z"). Updated valid_response.json (60 timestamps) and valid_equals_response.json (2 timestamps) to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…2316) Adds targeted tests across four layers to guard the millisecond-precision UNIX_TIMESTAMP sort key fix from silent regression: - go/types: threshold boundary (exact 1e11 semantics), zero/negative edge cases, ms-range list branch in ValueTypeToGoTypeTimestampAsString, and nil-safety of GetTimestampSeconds - cassandraonlinestore: resolveFeatureValue preserves time.Time unmodified; InterfaceToProtoValue encodes it as UnixMilli (not Unix seconds) - redisonlinestore: EventTimestamps in OnlineReadRange match the _ts field seconds value for each returned row - ScyllaDB integration: both sub-second regression tests now set IncludeMetadata=true and assert EventTimestamps are seconds-precision - Valkey integration: new sub-second fixture (regression_repo.py + sub_second_data.parquet) and TestGetOnlineFeaturesRangeValkey_SubSecondSortKey Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous fixture update applied .919Z sub-second suffix to ALL "2025-04-16 02:04:31Z" occurrences. That was only correct for the event_timestamp sort key feature values (02:06:xx.919Z, which are stored as ms). The array_timestamp_val element for 2025-04-16 is stored as whole seconds (1744769071), so it formats as "02:04:31Z" without sub-second suffix. Revert the 30 wrong entries in valid_response.json and 1 in valid_equals_response.json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
piket
changed the title
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_TIMESTAMPsort keys differed by less than onesecond 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 allUNIX_TIMESTAMPvalues to integer seconds before storing in
unix_timestamp_val. The* 1000multiplications in
zset_score()attempted ms precision but had no effect.What this PR does / why we need it
Fix: millisecond precision for UNIX_TIMESTAMP sort keys
_python_datetime_to_int_ms_timestamp()for millisecond-precision conversion._convert_arrow_fv_to_proto(), detectUNIX_TIMESTAMPsort-key columns and usethe ms function for those columns only; all other
UNIX_TIMESTAMPcolumns remain atsecond precision (backward-compatible).
* 1000fromzset_score()in Redis, Valkey, and Cassandra —unix_timestamp_valis now already in ms for sort-key columns.val > 1e11) infeast_value_type_to_python_type()so sort-key ms values round-trip correctly as datetimes without affecting the seconds
interpretation for regular feature columns.
Fix: disambiguate raw integer sort-key timestamps by magnitude
When a sort-key value arrives as a raw integer (e.g. already-materialized rows or
non-Arrow paths), distinguish seconds-since-epoch from milliseconds-since-epoch using
the same
> 1e11threshold so the read path handles both precisions consistently.Fix: Go serving layer for ms-precision UNIX_TIMESTAMP sort keys
Updated the Go feature-serving layer to apply the same magnitude-based disambiguation
when reading
UNIX_TIMESTAMPsort-key values back from the online store, ensuringGo-served results agree with the Python write path.
Feat: always-on version banner + row-count logging
get_version(): this fork publishes aseg-feast, notfeast, soimportlib.metadata.version("feast")silently returned"unknown"inall real deployments. Now checks both package names.
get_installed_version(package_name)for opportunistic lookup of wrapperpackages (e.g.
feature-store-materialization).materialize()/materialize_incremental()) and the streaming entry point(
SparkKafkaProcessor.ingest_stream_feature_view) now logfeast=<version> feature-store-materialization=<version>on startup,unconditionally.
_convert_arrow_to_proto()(the single choke point all three materialization writepaths converge on) now emits
[materialize] feature_view=<name> rows=<n>afterevery batch — no raw entity/feature data, no config flag required.
Which issue(s) this PR fixes
Fixes EAPC-22316
Testing
SortedFeatureViewwith a secondary custom-named sort key(
sort_key_2) to guard the multi-sort-key path.get_version()/get_installed_version()fallback behaviour(
test_version.py).test_type_map.py,test_feature_views.py,repo_config,online_store, andcompute_enginessuites(198 passed, 5 pre-existing skips).
Misc
Relates to the broader SortedFeatureView timestamp-precision work tracked in
EAPC-22316. The version-banner and row-count logging additions were motivated by
recurring "which build is actually running?" / "is the write path processing what I
think it is?" questions encountered during diagnosis.