Skip to content
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

[CALCITE-6703] RelJson cannot handle timestamps prior to 1970-01-25 2… #4060

Merged
merged 1 commit into from
Nov 22, 2024
Merged

[CALCITE-6703] RelJson cannot handle timestamps prior to 1970-01-25 2… #4060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,9 +830,16 @@ public RexNode toRex(RelOptCluster cluster, Object o) {
Sarg sarg = sargFromJson((Map) literal, type);
return rexBuilder.makeSearchArgumentLiteral(sarg, type);
}
if (type.getSqlTypeName() == SqlTypeName.SYMBOL) {
SqlTypeName sqlTypeName = type.getSqlTypeName();
if (sqlTypeName == SqlTypeName.SYMBOL) {
literal = RelEnumTypes.toEnum((String) literal);
}
if (sqlTypeName == SqlTypeName.TIMESTAMP
|| sqlTypeName == SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE) {
if (literal instanceof Integer) {
literal = ((Integer) literal).longValue();
}
}
return rexBuilder.makeLiteral(literal, type);
}
if (map.containsKey("sargLiteral")) {
Expand Down
55 changes: 55 additions & 0 deletions core/src/test/java/org/apache/calcite/plan/RelWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,61 @@ private static Fixture relFn(Function<RelBuilder, RelNode> relFn) {
.assertThatPlan(isLinux(expected));
}

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-6703">[CALCITE-6703]
* RelJson cannot handle timestamps prior to 1970-01-25 20:31:23.648</a>. */
@Test void testJsonToRexForTimestamp() {
// Below Integer.MAX_VALUE
final String timestampRepresentedAsInt = "{\n"
+ " \"literal\": 2129400000,\n"
+ " \"type\": {\n"
+ " \"type\": \"TIMESTAMP\",\n"
+ " \"nullable\": false\n"
+ " }\n"
+ "}\n";
// Above Integer.MAX_VALUE
final String timestampRepresentedAsLong = "{\n"
+ " \"literal\": 3129400000,\n"
+ " \"type\": {\n"
+ " \"type\": \"TIMESTAMP\",\n"
+ " \"nullable\": false\n"
+ " }\n"
+ "}\n";

// These timestamps were verified using BigQuery's UNIX_MILLIS function.
assertThatReadExpressionResult(timestampRepresentedAsInt, is("1970-01-25 15:30:00"));
Copy link
Contributor

Choose a reason for hiding this comment

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

is this supposed to work for dates before the epoch or the Gregorian calendar cutoff?
maybe you can add tests for these too - if they are not supposed to work, the errors can be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the mechanism going from millis->timestamp String actually lives in DateTimeUtils in Avatica and is pretty well-tested in DateTimeUtilsTest (let me know if you need links.) The real bug here is that JSON doesn't distinguish the numeric types so we have to ensure it's a Long (or convert) it. So that's the scope I'm trying to test here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have approved, I leave to you this decision.

assertThatReadExpressionResult(timestampRepresentedAsLong, is("1970-02-06 05:16:40"));
}

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-6703">[CALCITE-6703]
* RelJson cannot handle timestamps prior to 1970-01-25 20:31:23.648</a>. */
@Test void testJsonToRexForTimestampWithLocalTimeZone() {
// Below Integer.MAX_VALUE
final String timestampWithLocalTzRepresentedAsInt = "{\n"
+ " \"literal\": 2129400000,\n"
+ " \"type\": {\n"
+ " \"type\": \"TIMESTAMP_WITH_LOCAL_TIME_ZONE\",\n"
+ " \"nullable\": false\n"
+ " }\n"
+ "}\n";
// Above Integer.MAX_VALUE
final String timestampWithLocalTzRepresentedAsLong = "{\n"
+ " \"literal\": 3129400000,\n"
+ " \"type\": {\n"
+ " \"type\": \"TIMESTAMP_WITH_LOCAL_TIME_ZONE\",\n"
+ " \"nullable\": false\n"
+ " }\n"
+ "}\n";

// These timestamps were verified using BigQuery's UNIX_MILLIS function.
assertThatReadExpressionResult(timestampWithLocalTzRepresentedAsInt,
is("1970-01-25 15:30:00:TIMESTAMP_WITH_LOCAL_TIME_ZONE(0)"));
assertThatReadExpressionResult(timestampWithLocalTzRepresentedAsLong,
is("1970-02-06 05:16:40:TIMESTAMP_WITH_LOCAL_TIME_ZONE(0)"));
}


@Test void testJsonToRex() {
// Test simple literal without inputs
final String jsonString1 = "{\n"
Expand Down
Loading