Repository navigation
Conversation
pandas parses into nanoseconds, which reach only 1677 to 2262, so the exported script emptied every moment outside that window where the engine holds a java.sql.Timestamp and reads it like any other. It also inferred one format for the whole column, so a row written differently from the first one was emptied too, where the engine hands DateParserUtils a field at a time. The text branch now reads the column cell by cell at microsecond resolution. Text neither side can read still leaves an empty cell rather than ending the run, which is what the cast has always promised. Closes apache#8595 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 363 | 0.222 | 26,741/37,261/37,261 us | 🔴 +18.2% / 🔴 +155.1% |
| 🔴 | bs=100 sw=10 sl=64 | 767 | 0.468 | 126,909/163,976/163,976 us | 🔴 +13.4% / 🔴 +63.7% |
| 🟢 | bs=1000 sw=10 sl=64 | 908 | 0.554 | 1,102,901/1,199,715/1,199,715 us | 🟢 -7.8% / 🔴 +29.8% |
Baseline details
Latest main 5264df2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 363 tuples/sec | 411 tuples/sec | 912.36 tuples/sec | -11.7% | -60.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.222 MB/s | 0.251 MB/s | 0.557 MB/s | -11.6% | -60.1% |
| bs=10 sw=10 sl=64 | p50 | 26,741 us | 22,628 us | 11,391 us | +18.2% | +134.7% |
| bs=10 sw=10 sl=64 | p95 | 37,261 us | 34,187 us | 14,608 us | +9.0% | +155.1% |
| bs=10 sw=10 sl=64 | p99 | 37,261 us | 34,187 us | 17,250 us | +9.0% | +116.0% |
| bs=100 sw=10 sl=64 | throughput | 767 tuples/sec | 811 tuples/sec | 1,161 tuples/sec | -5.4% | -34.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.468 MB/s | 0.495 MB/s | 0.709 MB/s | -5.5% | -34.0% |
| bs=100 sw=10 sl=64 | p50 | 126,909 us | 122,407 us | 91,471 us | +3.7% | +38.7% |
| bs=100 sw=10 sl=64 | p95 | 163,976 us | 144,572 us | 100,192 us | +13.4% | +63.7% |
| bs=100 sw=10 sl=64 | p99 | 163,976 us | 144,572 us | 119,565 us | +13.4% | +37.1% |
| bs=1000 sw=10 sl=64 | throughput | 908 tuples/sec | 920 tuples/sec | 1,226 tuples/sec | -1.3% | -25.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.554 MB/s | 0.562 MB/s | 0.748 MB/s | -1.4% | -26.0% |
| bs=1000 sw=10 sl=64 | p50 | 1,102,901 us | 1,081,070 us | 887,282 us | +2.0% | +24.3% |
| bs=1000 sw=10 sl=64 | p95 | 1,199,715 us | 1,301,809 us | 924,352 us | -7.8% | +29.8% |
| bs=1000 sw=10 sl=64 | p99 | 1,199,715 us | 1,301,809 us | 951,858 us | -7.8% | +26.0% |
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,550.63,200,128000,363,0.222,26740.74,37261.49,37261.49
1,100,10,64,20,2609.02,2000,1280000,767,0.468,126909.24,163976.18,163976.18
2,1000,10,64,20,22016.35,20000,12800000,908,0.554,1102900.94,1199714.71,1199714.71
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8597 +/- ##
============================================
- Coverage 92.78% 92.56% -0.22%
- Complexity 4898 5060 +162
============================================
Files 1236 1245 +9
Lines 52121 53090 +969
Branches 6405 6542 +137
============================================
+ Hits 48359 49143 +784
- Misses 2186 2307 +121
- Partials 1576 1640 +64
*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:
|
carloea2
left a comment
There was a problem hiding this comment.
The new timestamp helper crashes on 2024-03-05T14:09:07Z. I ran the generated helper with pandas 2.2.3. Parsing keeps the timezone, then astype rejects the conversion to a timestamp without a timezone. The previous code accepts this value. Please handle timestamps with a timezone and add a test with Z and an explicit offset.
The cast stopped on one. A reading that carries a zone cannot be converted to a column that holds none, so `2024-03-05T14:09:07Z` ended the cast where the previous generator had read it, and an explicit offset did the same. The engine reads the offset and keeps no zone for it: DateParserUtils parses the reading and java.sql.Timestamp holds the wall clock of the machine's own zone, so `...T14:09:07Z` is 06:09:07 where the machine is eight hours behind UTC. The helper now does that, which is also what the epoch-milliseconds branch beside it already did. The two spellings join the cases the timestamp test compares against parseField cell by cell, so what they should read is taken from the engine rather than written down and the pair says the same thing in any zone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in |
carloea2
left a comment
There was a problem hiding this comment.
The timezone crash is fixed. I ran the current helper with Z, an explicit offset, year 2500, and null. Those checks passed.
carloea2
left a comment
There was a problem hiding this comment.
Reproduced locally with the generated Python code. One correctness issue below. I did not run a frontend workflow or the full Scala suite.
The cast narrowed one. A column the engine already holds as a moment went through the same astype as freshly parsed text, so 14:09:07.123456789 and 14:09:07.123456001 both came back as .123456 and two rows the run tells apart became one. The previous generator left such a column alone. parseField hands a java.sql.Timestamp back untouched and that class counts nanoseconds, so there is nothing for this branch to decide: the column is returned at the resolution it arrived in, zoned or not. The text branch below is unchanged and still reads into microseconds, which is what the years past the nanosecond edge need. A test casts a timestamp column to timestamp and compares the two values cell by cell against parseField, which is how the rest of the spec compares a cast. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… write text as Java's toString does Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@carloea2 Since your approval, text read into a timestamp is cut to the millisecond as DateParserUtils does, and a cast to text writes a value as Java's toString does. |
carloea2
left a comment
There was a problem hiding this comment.
Retested with JDK 17.0.20.1. The helper still writes 1.0E23 where the engine writes 9.999999999999999E22. The JDK 19 form would keep this mismatch. We need maintainer agreement on the supported formatting contract before accepting that difference.
The helper writes a double's shortest digits, as Double.toString does from JDK 19. On JDK 17 the engine sometimes writes more digits for the same value, so say so where the text is built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The access-control-service job could not resolve io.fabric8:kubernetes-client:6.12.1 before any test ran. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
The documented JDK 17 difference is accepted by Xinyuan, so that earlier finding is no longer a blocker. All 13 focused casting tests and scalafmtCheckAll pass locally. A separate Arrow NaN regression fails at this head.
…ring A nullable float column read from Arrow holds NaN as a value, but Series.map hands its empty cells over as NaN too, so the cast emptied a stored NaN the engine writes as "NaN". Read the empty cells from the column's own mask instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ne does The integer, long and boolean casts asked each value whether it was empty, and Series.apply hands a nullable float column's empty cells over as NaN, so a stored NaN came out null where the engine gives 0, 0 and true. They now read the column's own mask, as the string cast does. A double past an integer's bounds now saturates as toInt and toLong do, where int() raised on an infinity and stopped the script. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
The stored Arrow NaN fix passes. Two other cases remain in the generated casts: NaN parsed as DOUBLE becomes null on the next STRING cast, and a valid pre Unix epoch LONG crashes the timestamp cast on Windows. The existing 10 tests pass; my additional cases give 3 passes and 2 failures. Formatting passes.
sbt -batch 'WorkflowOperator/testOnly *TypeCastingOpDescSpec -- -oDF'
…pre-1970 milliseconds on Windows The double cast wrote float64, which holds a NaN and an empty cell alike, so text "NaN" cast to DOUBLE came out null at the next cast to STRING. It now builds a nullable Float64 from the column's own mask. Windows' localtime refuses an instant before 1970, so the LONG to TIMESTAMP cast stopped on one. Such a cell now takes the zone's standard offset, which is what java.sql.Timestamp writes before 1900. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous fix covered epoch milliseconds only. Text that states an offset and a column that states its zone also asked tzlocal() for the wall clock, so on Windows an instant before 1970 still stopped the cast. All three now go through one helper that falls back to the zone's standard offset, and a zoned column keeps its own resolution. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
The earlier NaN round trip and pre-1970 crash fixes pass: all 12 author tests and formatting checks are green. A historical timezone case still differs on Windows. At 1931-01-01T00:00:00Z in America/Mexico_City, the generated cast returns 1930-12-31 18:00; the native JVM returns 17:00. Modern, older-summer and null controls pass. The added historical regression fails. No production edits or dependency fixtures used.
sbt -batch 'WorkflowOperator/testOnly *TypeCastingOpDescSpec -- -oDF' 'scalafmtCheckAll'
sbt -batch 'WorkflowOperator/testOnly *TimestampHistoryReviewSpec -- -oDF'
…instant dateutil's tzlocal() knows only today's standard and daylight offsets, so an instant from before a zone changed its rules was read an hour or more away from the engine, on every platform. Read the machine's zone by its IANA name instead, through tzlocal, TZ or /etc/localtime, and keep today's standard offset before 1900 as java.util.Date does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pyamber hands a Python operator a DOUBLE column as float64, where a NaN and a null are one value, and writes a NaN out as null. The double cast here builds Float64, which keeps them apart, so a Python operator after it answered differently: Sort ordered a NaN apart from the nulls, a chart drew a point the engine dropped, and a passed-through NaN stayed NaN. StandaloneCodeGenerator gains standaloneInputAdapter(), and for a Python operator the translator feeds the fragment a converted copy of each input, leaving the upstream frame as it was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@carloea2 I added f0eedd9. The double cast now builds Float64, and a Python operator after it was handed a NaN that pyamber would have turned into null, so the translator now feeds Python operators a float64 copy the way pyamber does. I updated the description to cover this and the earlier rounds; could you take another look? |
carloea2
left a comment
There was a problem hiding this comment.
The historical timezone finding is fixed. On Windows with JDK 17, all 13 TypeCastingOpDescSpec tests, my 3 historical timezone regressions, and 13 translator tests pass against current upstream main. scalafmtCheckAll passes too. CI for this head is still queued.
Command: sbt -batch "WorkflowOperator/testOnly *TypeCastingOpDescSpec *TimestampHistoryReviewSpec" "WorkflowCompilingService/testOnly *WorkflowToPythonTranslatorSpec" 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>
What changes were proposed in this PR?
Type Casting to a timestamp read text with
pd.to_datetime(col, errors="coerce")in the exported script. That is wrong in two ways, and both empty a cell the run itself filled.pandas parses into nanoseconds, which reach only 1677-09-21 to 2262-04-11. The engine holds a
java.sql.Timestamp, where2500-01-01 00:00:00is an ordinary moment: it reads it, and the exported script answered with an empty cell. The same for1500-06-15 08:30:00and9999-12-31 23:59:59.pandas also infers one format for the whole column and coerces every row that does not match it. The engine hands
DateParserUtilsone field at a time, so a row states its own format. A column holding2024-03-05 14:09:07andMarch 5, 2024kept the first and emptied the second.The text branch now reads the column cell by cell and holds the result at microsecond resolution, which covers the years the engine covers. It still coerces: the engine accepts a set of formats no single pandas call states, so text neither side can read leaves an empty cell rather than ending an exported run halfway. That part is unchanged, and a test now says so.
Review and the verification's edge values found more places where the cast parted from the engine:
Zor+05:30, stopped the cast. The engine moves it to the machine's zone and keeps that wall clock, and the script now does the same.parseFieldreturns ajava.sql.Timestampuntouched, so the script now hands such a column back at the resolution it arrived in.DateParserUtilsreads into ajava.util.Date, which counts milliseconds, so the script now cuts there too.str()does. Java writes a double past 1e7 or under 1e-3 in E notation, a boolean in lower case, and a timestamp with its nanoseconds and at least.0. A shared_texera_java_textnow writes each one as Java'stoStringdoes.NaNas text, 0 as an integer andtrueas a boolean. The casts now read empty cells from the column's own mask, and the double cast builds a nullableFloat64, so a NaN read from text stays one through the next cast. A double past an integer's bounds now saturates astoIntandtoLongdo, whereint()raised on an infinity.tzlocal(), which knows only today's two offsets, so an instant from before a zone changed its rules came out an hour or more off on every platform. The script now reads the machine's zone by its IANA name and, before 1900, keeps today's standard offset asjava.util.Datedoes.localtimerefuses an instant before 1970, which stopped the cast. The same helper now falls back to the zone's standard offset there.That
Float64column changed what a Python operator after this cast is handed. pyamber hands a Python operator a DOUBLE column asfloat64, where a NaN and a null are one value, and writes a NaN out as null. GivenFloat64instead, Sort ordered a NaN apart from the nulls, a chart drew points the engine dropped, and a passed-through NaN stayed NaN.StandaloneCodeGeneratorgainsstandaloneInputAdapter(), and for a Python operator the translator feeds the fragment afloat64copy of each input, leaving the upstream frame as it was.The number branch is left alone. A LONG is read as epoch milliseconds and has the same window, but it also disagrees about the calendar before 1582-10-15, where
new Timestamp(long)is Julian and pandas is proleptic Gregorian. Widening the window there would answer with a moment eight days off instead of an empty cell, so what that branch should do is a separate decision. The issue records it.Any related issues, documentation, discussions?
Closes #8595. #8480 closed the same nanosecond gap for Extract Date/Time Fields, which parses strictly and could take the one-line form this operator cannot.
#8341, #8704, #8705 and #8709 call the timestamp and text helpers this adds, so this PR merges before them.
How was this PR tested?
Eight tests in
TypeCastingOpDescSpecrun the generated Python and compare it cell by cell againstAttributeTypeUtils.parseField. They are tagged to run in the integration job, which has pandas.2500-01-01,1500-06-15,9999-12-31, readings withZand+05:30, and readings past the millisecond. It fails on the old generator.AttributeTypeUtils, and check that a NaN read from text survives a second cast.America/Mexico_Cityin 1899, 1921, 1931 and 2021, at the offset the zone had then, and the same moments withlocaltimerefusing those before 1970.WorkflowToPythonTranslatorSpeccheck that a Python operator reads a converted copy of its input and an operator the engine runs on the JVM reads its upstream as it is. Two of them fail without the change.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)