Skip to content

Arrow Connector : Preserve TIME and TIMESTAMP as Wall-Clock (Align with JDBC Connector) #25901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lithinwxd wants to merge 6 commits into from
Closed

Arrow Connector : Preserve TIME and TIMESTAMP as Wall-Clock (Align with JDBC Connector) #25901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c631eab
Fix Time and Timestamp and time zone conversion issues for Arrow
lithinwxd Aug 27, 2025
8159a3d
Fix Time and Timestamp and time zone conversion issues for Arrow
lithinwxd Aug 27, 2025
c5f4adb
Merge branch 'prestodb:master' into feature_timestampFixForArrow
lithinwxd Aug 28, 2025
4415b48
Merge branch 'prestodb:master' into feature_timestampFixForArrow
lithinwxd Sep 17, 2025
d290d44
Merge branch 'prestodb:master' into feature_timestampFixForArrow
lithinwxd Sep 26, 2025
af5f46e
Merge branch 'prestodb:master' into feature_timestampFixForArrow
lithinwxd Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowBlockBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@
import java.math.BigDecimal;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.time.Instant;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -432,11 +436,25 @@ public void assignBlockFromTimeStampMilliVector(TimeStampMilliVector vector, Typ
}
Copy link
Copy Markdown
Contributor

@BryanCutler BryanCutler Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about assignBlockFromTimeStampMicroVector ?

else {
long millis = vector.get(i);
type.writeLong(builder, millis);
long localMillis = getLocalMillisFromUTCMillis(millis);
type.writeLong(builder, localMillis);
}
}
}

private static long getLocalMillisFromUTCMillis(long millis)
{
// Interpret Arrow millis as if they were "local wall-clock time" in datasource
LocalDateTime localDateTime = Instant.ofEpochMilli(millis)
.atZone(ZoneOffset.UTC) // treat raw millis as UTC
.toLocalDateTime();

// Rebuild millis in local epoch (wall-clock, no shift)
return localDateTime.atZone(ZoneId.systemDefault())
Comment thread
lithinwxd marked this conversation as resolved.
.toInstant()
.toEpochMilli();
}

public void assignBlockFromFloat8Vector(Float8Vector vector, Type type, BlockBuilder builder, int startIndex, int endIndex)
{
for (int i = startIndex; i < endIndex; i++) {
Expand Down Expand Up @@ -559,7 +577,9 @@ public void assignBlockFromTimeMilliVector(TimeMilliVector vector, Type type, Bl
}
else {
long millis = vector.get(i);
type.writeLong(builder, millis);
// Interpret Arrow millis as if they were "local wall-clock time" in datasource
long localMillis = getLocalMillisFromUTCMillis(millis);
Copy link
Copy Markdown
Contributor

@BryanCutler BryanCutler Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TimeMilliVector has no associated timezone, but here you are forcing it to be a UTC timezone, we shouldn't do this. We have to assume that a timestamp without a timezone is local time.

type.writeLong(builder, localMillis);
}
}
}
Expand Down Expand Up @@ -639,7 +659,9 @@ public void assignBlockFromTimeMilliTZVector(TimeStampMilliTZVector vector, Type
}
Copy link
Copy Markdown
Contributor

@BryanCutler BryanCutler Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TimeStampMilliTZVector has a specific TimeZone, you would need to use this instead of treating it as UTC. Actually, I think we should be converting to the presto data type TimestampWithTimeZoneType and use the TimeZone from the Arrow vector. Can you try this?

else {
long millis = vector.get(i);
type.writeLong(builder, millis);
// Interpret Arrow millis as if they were "local wall-clock time" in datasource
long localMillis = getLocalMillisFromUTCMillis(millis);
type.writeLong(builder, localMillis);
}
}
}
Expand Down
22 changes: 22 additions & 0 deletions presto-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/TestArrowBlockBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.charset.StandardCharsets;
import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -726,6 +730,15 @@ public void testAssignTimestampType()

Block block = builder.build();
long result = timestampType.getLong(block, 0);
// Recompute expected according to current reinterpretation logic
LocalDateTime localDateTime = Instant.ofEpochMilli(value)
.atZone(ZoneOffset.UTC) // interpret Arrow millis as UTC
.toLocalDateTime();

value = localDateTime
Copy link
Copy Markdown
Contributor

@BryanCutler BryanCutler Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't reuse value, make a new variable expected

.atZone(ZoneId.systemDefault()) // reinterpret in system default
.toInstant()
.toEpochMilli();
assertEquals(result, value);
}
}
Expand All @@ -748,6 +761,15 @@ public void testAssignTimestampTypeWithSqlTimestamp()

Block block = builder.build();
long result = timestampType.getLong(block, 0);
// Recompute expected according to current reinterpretation logic
LocalDateTime localDateTime = Instant.ofEpochMilli(expectedMillis)
.atZone(ZoneOffset.UTC) // interpret Arrow millis as UTC
.toLocalDateTime();

expectedMillis = localDateTime
Copy link
Copy Markdown
Contributor

@BryanCutler BryanCutler Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: change the definition of long expectedMillis to just long value and use expectedMillis here

.atZone(ZoneId.systemDefault()) // reinterpret in system default
.toInstant()
.toEpochMilli();
assertEquals(result, expectedMillis);
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...ow-flight/src/test/java/com/facebook/plugin/arrow/testingServer/TestingArrowProducer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public void getStream(CallContext callContext, Ticket ticket, ServerStreamListen

try (ResultSet resultSet = stmt.executeQuery(query.toUpperCase())) {
JdbcToArrowConfig config = new JdbcToArrowConfigBuilder().setAllocator(allocator).setTargetBatchSize(2048)
.setCalendar(Calendar.getInstance(TimeZone.getDefault())).build();
.setCalendar(Calendar.getInstance(TimeZone.getTimeZone("UTC"))).build();
Comment thread
lithinwxd marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@BryanCutler BryanCutler Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your assumption is correct. Timestamp vectors are represented as UTC instants in Arrow since arrow doesn't have timezone.

This is wrong, an Arrow TimeStampMilliVector does not assume UTC. It's interpretation is up to presto, and we have been using this as local time. You must use a TimeStampMilliTZVector and set the time zone to UTC, if you want a UTC values.

You shouldn't need to change this in the test, it is saying that our datasource is generating localtime data with a timezone, resulting in a TimeStampMilliTZVector with the system timezone.

Schema schema = jdbcToArrowSchema(resultSet.getMetaData(), config);
try (VectorSchemaRoot streamRoot = VectorSchemaRoot.create(schema, allocator)) {
VectorLoader loader = new VectorLoader(streamRoot);
Expand Down
Loading