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 | 534 | 0.326 | 19,158/22,745/22,745 us | 🟢 -35.9% / 🔴 +66.5% |
| 🔴 | bs=100 sw=10 sl=64 | 1,180 | 0.72 | 80,038/116,068/116,068 us | 🔴 +29.6% / 🔴 +20.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,441 | 0.879 | 690,773/783,276/783,276 us | ⚪ within ±5% / 🟢 +21.9% |
Baseline details
Latest main 5264df2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 534 tuples/sec | 516 tuples/sec | 879.16 tuples/sec | +3.5% | -39.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.326 MB/s | 0.315 MB/s | 0.537 MB/s | +3.5% | -39.2% |
| bs=10 sw=10 sl=64 | p50 | 19,158 us | 18,010 us | 11,504 us | +6.4% | +66.5% |
| bs=10 sw=10 sl=64 | p95 | 22,745 us | 35,488 us | 14,234 us | -35.9% | +59.8% |
| bs=10 sw=10 sl=64 | p99 | 22,745 us | 35,488 us | 17,338 us | -35.9% | +31.2% |
| bs=100 sw=10 sl=64 | throughput | 1,180 tuples/sec | 1,274 tuples/sec | 1,147 tuples/sec | -7.4% | +2.8% |
| bs=100 sw=10 sl=64 | MB/s | 0.72 MB/s | 0.778 MB/s | 0.7 MB/s | -7.5% | +2.8% |
| bs=100 sw=10 sl=64 | p50 | 80,038 us | 79,254 us | 88,987 us | +1.0% | -10.1% |
| bs=100 sw=10 sl=64 | p95 | 116,068 us | 89,554 us | 96,473 us | +29.6% | +20.3% |
| bs=100 sw=10 sl=64 | p99 | 116,068 us | 89,554 us | 102,791 us | +29.6% | +12.9% |
| bs=1000 sw=10 sl=64 | throughput | 1,441 tuples/sec | 1,453 tuples/sec | 1,182 tuples/sec | -0.8% | +21.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.879 MB/s | 0.887 MB/s | 0.721 MB/s | -0.9% | +21.9% |
| bs=1000 sw=10 sl=64 | p50 | 690,773 us | 688,900 us | 872,751 us | +0.3% | -20.9% |
| bs=1000 sw=10 sl=64 | p95 | 783,276 us | 801,039 us | 910,151 us | -2.2% | -13.9% |
| bs=1000 sw=10 sl=64 | p99 | 783,276 us | 801,039 us | 941,479 us | -2.2% | -16.8% |
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,374.78,200,128000,534,0.326,19157.93,22744.68,22744.68
1,100,10,64,20,1695.40,2000,1280000,1180,0.720,80038.01,116067.70,116067.70
2,1000,10,64,20,13882.44,20000,12800000,1441,0.879,690773.43,783275.51,783275.51
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8597 +/- ##
============================================
- Coverage 92.78% 91.73% -1.06%
- Complexity 4898 5073 +175
============================================
Files 1236 1249 +13
Lines 52121 53859 +1738
Branches 6405 6658 +253
============================================
+ Hits 48359 49405 +1046
- Misses 2186 2772 +586
- Partials 1576 1682 +106
*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>
_texera_text_to_timestamp answered unreadable text with an empty cell. The engine's parseField throws on the same text and ends the run, so the script now raises the engine's message instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The script now raises where the engine's parseField throws, so the spec asserts that both refuse "not a date" instead of an empty cell. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@carloea2 Since your approval I pushed two commits. A timestamp cast now raises on text no format reads, as the engine's |
carloea2
left a comment
There was a problem hiding this comment.
Retested the timestamp refusal change on fresh upstream main. All 13 TypeCastingOpDescSpec tests and 13 WorkflowToPythonTranslatorSpec tests pass, including invalid text and the input adapter. scalafmtCheckAll passes. Code looks good. The current CI failures occur before tests because the Ampel action is not allowed.
A helper block serves several operators, so a plan pulled in every function the block defined. CSV File Scan followed by Filter carried the whole type-cast transcription, about 170 lines the script never called. The translator now writes the operator code first, follows the _texera_ calls from it through the helpers, and emits only those definitions. Anything in a block that is not such a definition is kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rted script The type-cast helpers carried their reasoning as Python comments, so every exported script that used them printed about 90 lines of notes written for whoever maintains the export. The notes now live in the Scaladoc, one line per function, and the Python has none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…of the exported script Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ne shared helper 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. Text no format reads now ends the run, as the engine's
parseFieldends its own. The script raises the engine's message,Failed to parse type str to Timestamp, where it used to leave the cell empty. dateutil reads every format the verification hands the engine, so the two refuse the same text.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)