Skip to content

feat(workflow-operator): compare Filter's exported predicates the way the engine does - #8980

Draft
kz930 wants to merge 9 commits into
apache:mainfrom
kz930:fix/8977-filter-engine-comparisons
Draft

kz930 wants to merge 9 commits into
apache:mainfrom
kz930:fix/8977-filter-engine-comparisons

Conversation

@kz930

@kz930 kz930 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Filter's exported Python compared the column to the value with pandas operators. The engine's FilterPredicate.evaluate compares by the column's type instead, so the two disagreed on numbers stored as text, booleans, NaN and -0.0.

Each comparison in the export now goes through _texera_filter_keep, a transcription of FilterPredicate.evaluate:

Column type Comparison
STRING as Java doubles when both sides parse as one, otherwise by UTF-16 text
BOOLEAN the lower-case text "true"/"false"
INTEGER, DOUBLE by Double.compareTo: NaN equals NaN and sorts above every number, -0.0 sorts below 0.0; a hex value past the largest double is infinity, as in Java
LONG as integers, rejecting what Long.valueOf rejects
TIMESTAMP by whole milliseconds; a value with an offset is read as the wall clock it shows in the machine's zone, with the offset that zone had at that instant

The column type comes from the input schema. Without one, it is inferred from the pandas dtype. is null and is not null are unchanged.

One approximation remains. The engine reads a TIMESTAMP value with DateParserUtils, which accepts more formats than pandas. A value pandas cannot read raises an error, so the export never gives a silently wrong answer.

A value with an offset is placed in the machine's zone by _texera_local_wall_clock from #8597. It uses the zone's IANA history, as the JVM does, and not Windows' localtime. Before 1900 it takes the zone's standard offset, as the engine does.

Any related issues, documentation, discussions?

Closes #8977

Depends on #8597, which adds _texera_local_wall_clock, and on #9035, which reads a column named in a different case. This PR stays a draft until both merge.

How was this PR tested?

A new test in SpecializedFilterOpDescSpec runs 33 predicates across the six column types through SpecializedFilterOpExec and through the export, and compares the kept rows by input position. A second test does the same for timestamp values with an offset, with both the JVM and Python set to Asia/Kolkata. Both fail on main and pass with this change. A third test sets both to America/Mexico_City and compares 2021-07-01T00:00:00Z (summer time) and 1931-01-01T00:00:00Z (an older offset). With #8597 merged in, the Filter specs pass (42 tests). A fourth test compares a LONG column Key through a predicate on KEY, past 2^53, and passes with #8597 and #9035 merged in.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5.5)

🤖 Generated with Claude Code

…does

The export compared the column to the value with pandas operators, while
FilterPredicate compares by type: a STRING field and the value as numbers
when both parse as one, a BOOLEAN as its lower-case text, an INTEGER as a
double, a DOUBLE with Double.compareTo, and a TIMESTAMP by getTime. Each
comparison now runs through a transcription of FilterPredicate.evaluate.

Closes apache#8977

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

@codecov-commenter

codecov-commenter commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.84%. Comparing base (6a29c59) to head (4de04d3).
⚠️ Report is 63 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...mber/operator/filter/SpecializedFilterOpDesc.scala 50.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8980      +/-   ##
============================================
- Coverage     92.61%   91.84%   -0.78%     
+ Complexity     5059     5049      -10     
============================================
  Files          1258     1258              
  Lines         53780    54308     +528     
  Branches       6707     6778      +71     
============================================
+ Hits          49811    49879      +68     
- Misses         2313     2752     +439     
- Partials       1656     1677      +21     
Flag Coverage Δ *Carryforward flag
access-control-service 77.44% <ø> (+0.06%) ⬆️
agent-service 99.16% <ø> (ø) Carriedforward from c21c9a2
amber 86.03% <50.00%> (-2.08%) ⬇️ Carriedforward from c21c9a2
computing-unit-managing-service 61.84% <ø> (+1.35%) ⬆️
config-service 87.50% <ø> (+0.12%) ⬆️
file-service 82.00% <ø> (ø) Carriedforward from c21c9a2
frontend 96.59% <ø> (ø) Carriedforward from c21c9a2
notebook-migration-service 83.77% <ø> (+0.03%) ⬆️
pyamber 98.58% <ø> (ø) Carriedforward from c21c9a2
workflow-compiling-service 72.61% <ø> (+0.17%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 6a29c59 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🟢 bs=10 sw=10 sl=64 436 0.266 22,087/31,083/31,083 us 🟢 -21.6% / 🔴 +154.3%
🔴 bs=100 sw=10 sl=64 923 0.564 103,404/150,783/150,783 us 🔴 +10.5% / 🔴 +73.0%
⚪ bs=1000 sw=10 sl=64 1,093 0.667 909,395/986,341/986,341 us ⚪ within ±5% / 🔴 -22.8%
Baseline details

Latest main 6a29c59 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 436 tuples/sec 451 tuples/sec 1,019 tuples/sec -3.3% -57.2%
bs=10 sw=10 sl=64 MB/s 0.266 MB/s 0.275 MB/s 0.622 MB/s -3.3% -57.2%
bs=10 sw=10 sl=64 p50 22,087 us 21,390 us 10,144 us +3.3% +117.7%
bs=10 sw=10 sl=64 p95 31,083 us 39,646 us 12,225 us -21.6% +154.3%
bs=10 sw=10 sl=64 p99 31,083 us 39,646 us 15,527 us -21.6% +100.2%
bs=100 sw=10 sl=64 throughput 923 tuples/sec 986 tuples/sec 1,371 tuples/sec -6.4% -32.7%
bs=100 sw=10 sl=64 MB/s 0.564 MB/s 0.602 MB/s 0.837 MB/s -6.3% -32.6%
bs=100 sw=10 sl=64 p50 103,404 us 98,203 us 79,910 us +5.3% +29.4%
bs=100 sw=10 sl=64 p95 150,783 us 136,501 us 87,155 us +10.5% +73.0%
bs=100 sw=10 sl=64 p99 150,783 us 136,501 us 106,643 us +10.5% +41.4%
bs=1000 sw=10 sl=64 throughput 1,093 tuples/sec 1,122 tuples/sec 1,415 tuples/sec -2.6% -22.8%
bs=1000 sw=10 sl=64 MB/s 0.667 MB/s 0.685 MB/s 0.864 MB/s -2.6% -22.8%
bs=1000 sw=10 sl=64 p50 909,395 us 886,822 us 780,407 us +2.5% +16.5%
bs=1000 sw=10 sl=64 p95 986,341 us 942,611 us 817,814 us +4.6% +20.6%
bs=1000 sw=10 sl=64 p99 986,341 us 942,611 us 848,683 us +4.6% +16.2%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,458.81,200,128000,436,0.266,22087.33,31082.88,31082.88
1,100,10,64,20,2166.14,2000,1280000,923,0.564,103403.82,150783.25,150783.25
2,1000,10,64,20,18291.65,20000,12800000,1093,0.667,909395.32,986340.77,986340.77

@kz930

kz930 commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Could you review this? Filter's export now compares each predicate by the column's type, as FilterPredicate.evaluate does, and a new spec test checks 26 predicates against the engine.

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 40 existing Filter tests and scalafmtCheckAll pass against current upstream main on JDK 17. Two additional native versus export parity cases fail below. Finite hexadecimal, unzoned timestamp and null controls pass.

Commands: sbt -batch "WorkflowOperator/testOnly *SpecializedFilterOpDescSpec *FilterPredicateSpec *SpecializedFilterOpExecSpec" scalafmtCheckAll; sbt -batch "WorkflowOperator/testOnly *FilterBoundaryReviewSpec"

…flow as the engine does

An offset-bearing timestamp value is an instant, which the engine holds as
the wall clock it shows in the machine's zone, so the export now converts it
there before comparing instead of raising on a naive column. A hex double
past the largest double is infinity in Java, so the export no longer lets
float.fromhex raise OverflowError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930

kz930 commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Thanks, both findings are fixed in 55ce604 with regressions against the native executor. Could you take another look?

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both original regressions now pass. A remaining Windows timezone problem is reproduced below. Of 47 focused tests, 44 pass and 3 fail: the two historical cases and the new Asia/Kolkata test, whose TZ setting is not honored as an IANA zone by Windows localtime. scalafmtCheckAll passes.

Command: sbt -batch "WorkflowOperator/testOnly *SpecializedFilterOpDescSpec *FilterPredicateSpec *SpecializedFilterOpExecSpec *FilterBoundaryReviewSpec" scalafmtCheckAll

…offset

A timestamp value with an offset now becomes the machine-zone wall clock
through apache#8597's _texera_local_wall_clock, which uses the zone's IANA history
instead of Windows localtime. Adds a Mexico City test for 2021 summer time
and 1931.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930
kz930 marked this pull request as draft October 16, 2026 09:24

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The historical timezone finding is fixed. With #8597 at f0eedd9 merged locally into current upstream main, all 61 focused tests and scalafmtCheckAll pass on Windows. That includes my original hexadecimal, offset and historical regressions. The PR head alone lacks _texera_local_zone, so the CI dependency is real. Leaving the draft unchanged until #8597 lands and CI passes.

Command: sbt -batch "WorkflowOperator/testOnly *SpecializedFilterOpDescSpec *FilterPredicateSpec *SpecializedFilterOpExecSpec *FilterBoundaryReviewSpec *TypeCastingOpDescSpec" scalafmtCheckAll

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested together with #8597 and #9035 after resolving only the import conflicts. Nine existing tests pass. The #9035 generated-code assertion needs updating for the new helper, and an added native-versus-export regression finds a LONG comparison mismatch.

sbt -batch 'WorkflowOperator/testOnly *SpecializedFilterOpDescSpec -- -oDF'

kz930 and others added 4 commits October 23, 2026 14:43
…insensitive lookup

The engine finds a predicate's field through Schema, which ignores case. An
exact-name lookup found no type for a column named in another case, fell back
to the dtype, and compared a LONG as a double.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ported script

_texera_filter_keep explained each comparison in Python comments, which
every exported script with a Filter printed. The rules now sit in the
Scaladoc, one sentence per type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… type needs

_texera_filter_keep held every type's comparison in one function, so a
Filter on a DOUBLE column carried the LONG, BOOLEAN, TIMESTAMP and STRING
code too. Each type now has its own function, the generated code calls
the one its column's declared type needs, and the translator drops the
rest. Without a schema, _texera_filter_keep still picks one by dtype.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…umber parsers

Filter kept its own String.trim, Double.parseDouble and a LONG check
that matched Long.parseLong. It now calls _texera_java_trim,
_texera_java_double and _texera_parse_int from AttributeCasts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LONG Key and KEY regression is fixed. All 12 SpecializedFilterOpDescSpec tests pass with the #9035 lookup API and timestamp helpers supplied. The native filter and export now keep only the correct row beyond 2^53. scalafmtCheckAll passes. The combined #9035 generated-code assertion was updated mechanically for the new helper. CI is still red, so the dependencies and checks need clearing before merge.

kz930 and others added 2 commits October 24, 2026 14:54
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930

kz930 commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Since your review, Filter orders doubles with the shared _texera_double_key and its helpers are spaced like the rest of the export. Could you take another look?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exported Python for Filter disagrees with the engine on numbers in text, booleans, NaN and -0.0

3 participants