fix(substrait): decode deprecated timestamp literals as microseconds - #8832
Open
jonasdedden wants to merge 1 commit into
Open
fix(substrait): decode deprecated timestamp literals as microseconds#8832jonasdedden wants to merge 1 commit into
jonasdedden wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The Rust decode-boundary normalization matches Substrait’s microsecond contract for deprecated literals and fixes existing PyArrow inputs without duplicating semantics in Python. It preserves current precision literals and legacy non-default unit mappings; focused Rust and end-to-end PyArrow tests cover naive and timezone-aware comparisons.
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.
Fixes #8833. Also fixes #2514, which is the same bug reached through DuckDB and was closed without a fix.
Problem
A PyArrow filter on a timestamp column returned the wrong rows and raised nothing.
pc.field("ts") > pa.scalar(datetime(2024, 1, 3, 2), pa.timestamp("us"))matched 0 of 100 rows where 49 was correct; the same filter written as SQL was fine. Timezone-aware columns failed the scan instead of answering wrongly.Cause
PyArrow encodes the literal as Substrait's deprecated
Literal.timestamp, defined by the spec as microseconds since the epoch, and leavestype_variation_referenceat 0. DataFusion's consumer takes that field's unit fromtype_variation_referenceand maps 0 to seconds, so the literal arrived a million times too large. Ontimestamp[us]the following cast overflows to null, which is why>and<both returned nothing. The deprecatedLiteral.timestamp_tzhas no consumer branch at all.Change
Before handing the expression to DataFusion, rewrite both deprecated literals into
precision_timestamp/precision_timestamp_tz, which state the unit rather than implying it. Only the default reference changes meaning: references 1, 2 and 3 keep the milli/micro/nano units DataFusion gives them, and any other reference is left alone so DataFusion still reports it. Lance's own encode path uses the current DataFusion producer, which emitsprecision_timestamp, so it is unaffected.remap_expr_referencesis renamed tonormalize_exprbecause it now does more than remap field references. PyArrow could also be changed to emit the newer encoding; this handles the plans it produces today.Tests
rust/lance-datafusion/src/substrait.rs: parses the deprecated literal at each variation reference and asserts the resulting unit, plus the tz form that used to fail.python/python/tests/test_filter.py:>,<and==againsttimestamp[s|ms|us], naive and with a timezone, checked against PyArrow's own answer.The DuckDB query from #2514 returns the matching row on this branch and an empty frame on pylance 10.0.0.