Repository navigation
Conversation
…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>
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is
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
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| 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|
@carloea2 Could you review this? Filter's export now compares each predicate by the column's type, as |
carloea2
left a comment
There was a problem hiding this comment.
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>
carloea2
left a comment
There was a problem hiding this comment.
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>
carloea2
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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'
…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
left a comment
There was a problem hiding this comment.
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.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@carloea2 Since your review, Filter orders doubles with the shared |
What changes were proposed in this PR?
Filter's exported Python compared the column to the value with pandas operators. The engine's
FilterPredicate.evaluatecompares 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 ofFilterPredicate.evaluate: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 JavaLong.valueOfrejectsThe column type comes from the input schema. Without one, it is inferred from the pandas dtype.
is nullandis not nullare 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_clockfrom #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
SpecializedFilterOpDescSpecruns 33 predicates across the six column types throughSpecializedFilterOpExecand 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 compares2021-07-01T00:00:00Z(summer time) and1931-01-01T00:00:00Z(an older offset). With #8597 merged in, the Filter specs pass (42 tests). A fourth test compares a LONG columnKeythrough a predicate onKEY, 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