Encode non-negative nanos for negative ORC timestamps

[vuule]

Description

Fixes #19350.

The ORC writer split negative timestamps with a fractional second into a negative nanos remainder, which was then stored in the unsigned SECONDARY stream as a large value. The libcudf reader round-tripped this correctly, but Apache ORC readers (e.g. Spark) failed with nanos > 999999999 or < 0.

The writer now emits the same (seconds, nanos) pair as the Apache ORC writer: floor seconds with a non-negative nanos remainder, plus the second that Apache readers borrow back when the stored seconds are negative and the stored nanos are at least 1 ms (ORC-306/ORC-763). Existing files remain readable.

One consequence is inherited from the format: timestamps within 999 ms before the epoch are stored with zero seconds, so no reader can tell them apart from the same nanos one second later, and they read back one second too late. Apache ORC has the same limitation and asserts it in its own tests (ORC-763, ORC-771); OrcWriterTest.NegativeTimestampsNearEpoch pins the behavior for libcudf.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[res-life]

Thanks for working on this. I tested the current PR head against the original values from #19350 and additional near-epoch boundaries.

The good news is that the original #19350 values written by libcudf can now be read by Apache ORC without IllegalArgumentException: nanos > 999999999 or < 0, and all five values match exactly.

However, the current implementation introduces a libcudf ORC round-trip correctness regression for negative timestamps in [-999000us, -1us]: they are shifted forward by exactly one second.

[P1] Preserve near-epoch negative timestamps — cpp/src/io/orc/stripe_enc.cu:818

This focused C++ test can be added after test_negative_fractional_timestamp_roundtrip:

TEST_F(OrcWriterTest, NegativeTimestampWithinOneSecondOfEpoch)
{
  test_negative_fractional_timestamp_roundtrip<cudf::timestamp_us>(
    {-1L, -500L, -500'000L, -999'000L, -999'001L, -999'999L, -1'000'001L, -5'999'500L});
}

With this PR, the test reports the equivalent of:

expected=[-1, -500, -500000, -999000, -999001, -999999, -1000001, -5999500]
actual  =[999999, 999500, 500000, 1000, -999001, -999999, -1000001, -5999500]

The same case can be reproduced through the Java bindings by adding this to TableTest.java:

@Test
void testORCNegativeTimestampWithinOneSecondOfEpoch() throws IOException {
  long[] values = {
      -1L,
      -500L,
      -500_000L,
      -999_000L,
      -999_001L,
      -999_999L,
      -1_000_001L,
      -5_999_500L
  };

  try (TempFile tempFile = TempFile.create("near-epoch", ".orc");
       ColumnVector timestamps = ColumnVector.timestampMicroSecondsFromLongs(values);
       Table expected = new Table(timestamps)) {
    File file = tempFile.getFile();
    ORCWriterOptions writeOptions = ORCWriterOptions.builder()
        .withNonNullableColumns("ts")
        .build();

    try (TableWriter writer = Table.writeORCChunked(writeOptions, file)) {
      writer.write(expected);
    }

    ORCOptions readOptions = ORCOptions.builder()
        .withTimeUnit(DType.TIMESTAMP_MICROSECONDS)
        .build();
    try (Table actual = Table.readORC(readOptions, file)) {
      assertTablesAreEqual(expected, actual);
    }
  }
}

I also checked the behavior before this PR: the libcudf GPU write/read round trip preserves these near-epoch values exactly, while an Apache ORC reader fails on the old negative-nanos encoding. So this is a regression introduced by the normalization in this PR, rather than pre-existing libcudf reader behavior.

A simple seconds == 0 adjustment does not fix it: it changes -1us to -1000001us. This needs an encoding/reader compatibility strategy that preserves both Apache ORC interoperability and libcudf round-trip correctness.

[vuule]

Thanks for testing this so thoroughly. The one second shift you found for [-999000us, -1us] isn't something the writer can avoid: those timestamps are not representable in ORC, and Apache ORC produces exactly the values you listed for the same input.

ORC stores a timestamp as (seconds, nanos) with nanos in [0, 1e9), and every Apache reader borrows a second only when the stored seconds are negative and the stored nanos are at least 1 ms (Java TreeReaderFactory.TimestampTreeReader.readTimestamp, C++ TimestampColumnReader::next):

if (millis < 0 && newNanos > 999_999) { millis -= TimestampTreeWriter.MILLIS_PER_SECOND; }

The Apache writers cancel that borrow on write (if (secs < 0 && nanos > 999999) secs += 1), so -500us is stored as seconds = 0, nanos = 999500000. The stored seconds are not negative, so no reader borrows and the value comes back as +999500us. Working through the decode, there is no (seconds, nanos) pair that decodes to any value in [-999ms, -1ns].

Apache treats this as a known limitation of the format rather than a bug to fix, and asserts it in TestVectorOrcFile.testTimestampBug (added by ORC-771 after the ORC-763 discussion):

if (seconds[r] == -1) {
  // reproduce the JDK bug of java.sql.Timestamp see ORC-763
  // Wrong extra second: 1969-12-31 23.59.59.001 -> 1970-01-01 00.00.00.001
  assertEquals(0, timestamps.getTimestampAsLong(r));
}

I ran the external-reader check you asked for: wrote your values with this PR and read the file back with the Apache ORC C++ reader (via pyarrow). No exception, and the result matches your list exactly.

written (us)	Apache ORC reads (us)	delta
-1	999999	+1 s
-500	999500	+1 s
-500000	500000	+1 s
-999000	1000	+1 s
-999001	-999001	0
-999999	-999999	0
-1000001	-1000001	0
-5999500	-5999500	0

So libcudf and Apache now agree on every value here, which I believe is the property that matters for Spark: a file written on the GPU decodes to the same values as a file written by CPU Spark from the same input. The alternative that keeps the libcudf round trip lossless for this range is the old negative-nanos encoding, which is precisely what makes Apache readers throw nanos > 999999999 or < 0 and what makes GPU and CPU results differ.

Note that the affected range is [-999ms, -1ns]; -999000001ns and earlier are stored with negative seconds and round trip losslessly, which is why -999001us in your list is fine.

Updated in the latest commit: the writer is now a direct transcription of the Apache rule (floor seconds and non-negative nanos, then give back the second the reader borrows), and your near-epoch values are covered by a new OrcWriterTest.NegativeTimestampsNearEpoch test that pins the one second shift as documented behavior, next to the lossless cases in OrcWriterTest.NegativeFractionalTimestamps. A lossless representation for that range would have to come from a format/reader change upstream in Apache ORC.

[vuule]

AI took the liberty to reply with a detailed explanation of the findings, but I haven't checked them myself 🙈
Still, the code changes could fix the issue.
@res-life, can you check if the latest update changed the behavior for you?

[res-life]

This PR now matches Apache ORC Java's timestamp encoding exactly.

However, please note that it also inherits a bug from Apache ORC Java. For timestamp values in the range [-999 ms, 0), the sign is lost during encoding, and the values are read back one second later:

Original	Written (seconds, nanos)	Read
-500 µs	(0, 999500000)	+999500 µs
-1 ns	(0, 999999999)	+999999999 ns
-999 ms	(0, 1000000)	+1 ms

The original negative value cannot be restored because its encoded (seconds, nanos) representation is identical to that of the corresponding positive timestamp.

I am OK with this PR because cudf-spark should match Apache ORC Java's behavior, including this known limitation.