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 | 486 | 0.297 | 19,196/28,859/28,859 us | 🟢 -9.0% / 🔴 +82.6% |
| 🔴 | bs=100 sw=10 sl=64 | 965 | 0.589 | 102,208/135,723/135,723 us | 🟢 -9.9% / 🔴 +24.8% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,160 | 0.708 | 854,478/987,316/987,316 us | ⚪ within ±5% / 🟢 -14.5% |
Baseline details
Latest main 5264df2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 486 tuples/sec | 474 tuples/sec | 767.43 tuples/sec | +2.5% | -36.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.297 MB/s | 0.29 MB/s | 0.468 MB/s | +2.4% | -36.6% |
| bs=10 sw=10 sl=64 | p50 | 19,196 us | 19,312 us | 12,880 us | -0.6% | +49.0% |
| bs=10 sw=10 sl=64 | p95 | 28,859 us | 31,700 us | 15,801 us | -9.0% | +82.6% |
| bs=10 sw=10 sl=64 | p99 | 28,859 us | 31,700 us | 19,767 us | -9.0% | +46.0% |
| bs=100 sw=10 sl=64 | throughput | 965 tuples/sec | 1,005 tuples/sec | 988.73 tuples/sec | -4.0% | -2.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.589 MB/s | 0.613 MB/s | 0.603 MB/s | -3.9% | -2.4% |
| bs=100 sw=10 sl=64 | p50 | 102,208 us | 94,044 us | 102,684 us | +8.7% | -0.5% |
| bs=100 sw=10 sl=64 | p95 | 135,723 us | 150,699 us | 108,712 us | -9.9% | +24.8% |
| bs=100 sw=10 sl=64 | p99 | 135,723 us | 150,699 us | 118,731 us | -9.9% | +14.3% |
| bs=1000 sw=10 sl=64 | throughput | 1,160 tuples/sec | 1,202 tuples/sec | 1,022 tuples/sec | -3.5% | +13.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.708 MB/s | 0.734 MB/s | 0.624 MB/s | -3.5% | +13.5% |
| bs=1000 sw=10 sl=64 | p50 | 854,478 us | 825,197 us | 999,086 us | +3.5% | -14.5% |
| bs=1000 sw=10 sl=64 | p95 | 987,316 us | 970,047 us | 1,037,033 us | +1.8% | -4.8% |
| bs=1000 sw=10 sl=64 | p99 | 987,316 us | 970,047 us | 1,066,123 us | +1.8% | -7.4% |
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,411.11,200,128000,486,0.297,19195.84,28859.14,28859.14
1,100,10,64,20,2071.69,2000,1280000,965,0.589,102207.62,135723.30,135723.30
2,1000,10,64,20,17239.71,20000,12800000,1160,0.708,854478.01,987315.79,987315.79
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8597 +/- ##
============================================
- Coverage 92.78% 92.77% -0.01%
Complexity 4898 4898
============================================
Files 1236 1236
Lines 52121 52131 +10
Branches 6405 6407 +2
============================================
+ Hits 48359 48365 +6
- Misses 2186 2189 +3
- Partials 1576 1577 +1
*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
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
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
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.
| | if pd.api.types.is_datetime64_any_dtype(s): | ||
| | if getattr(s.dtype, "tz", None) is not None: | ||
| | s = s.dt.tz_convert(tzlocal()).dt.tz_localize(None) | ||
| | return s.astype("datetime64[us]") |
There was a problem hiding this comment.
This also converts existing timestamp columns to microseconds. I tested 14:09:07.123456789 and 14:09:07.123456001: both become 14:09:07.123456, while the previous export keeps them distinct. Please preserve the input resolution for timestamp-to-timestamp casts and add a nanosecond test.
There was a problem hiding this comment.
Confirmed and fixed in ca67ce1. The astype was meant for the text path and was also catching an already-parsed column; that branch now returns the column at the resolution it arrived in, and a test compares your two values against parseField cell by cell.
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>
3 participants
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.
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.
How was this PR tested?
Two tests in
TypeCastingOpDescSpec, both running the generated Python and comparing cell by cell againstAttributeTypeUtils.parseField, which is how that spec already compares the other casts.The first reads eight moments in one column: three ordinary ones written in three different formats, the two rows either side of the nanosecond edge, and
2500-01-01,1500-06-15and9999-12-31. It fails on the old generator, which empties five of the eight. The second says the cast still answers unreadable text with an empty cell, where the engine refuses it.The whole
WorkflowOperatormodule passes, 2567 tests.scalafmtCheckAllandscalafixAll --checkare clean.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)