You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/05/27 09:57:52 UTC
[GitHub] [calcite] wenhuitang opened a new pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,he query will report an error
wenhuitang opened a new pull request #1990:
URL: https://github.com/apache/calcite/pull/1990
PR for issue https://issues.apache.org/jira/browse/CALCITE-4026
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r435075978
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +174,13 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ return literal.getValue2();
}
Review comment:
For timestamp types, the issue may exist in many places, including Avatica. So I'm inclined to open an umbrella JIRA and fix them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r435785278
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +180,26 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ getDateFormatter(TIMESTAMP3_FORMAT_WITH_TIME_ZONE);
Review comment:
That seems to be better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r442085701
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
How about ISO_DATETIME_SECONDS_FORMAT_STRING and ISO_DATETIME_SECONDS_WITH_DECIMAL_FRACTION_FORMAT_STRING?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r432252061
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +179,28 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ String dateTimeFormatISO = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ new SimpleDateFormat(dateTimeFormatISO, Locale.ROOT);
+ dateFormatter.setTimeZone(DateTimeUtils.UTC_ZONE);
Review comment:
Are there any codes we can reuse in `DataTimeUtils` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r440130136
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +180,26 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ getDateFormatter(TIMESTAMP_FORMAT_STRING2);
+ return dateFormatter.format(literal.getValue2());
+ case DATE:
+ assert value instanceof DateString;
+ return value.toString();
+ default:
+ return literal.getValue3();
+ }
Review comment:
Why change from `getValue2()` to `getValue3()` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r442085701
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
How about ISO_DATETIME_SECONDS_FORMAT_STRING and ISO_DATETIME_SECONDS_WITH_DECIMAL_FRACTION_FORMAT_STRING?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r440130813
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
Can we use more readable name for these formats ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r435070231
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +180,26 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ getDateFormatter(TIMESTAMP3_FORMAT_WITH_TIME_ZONE);
Review comment:
The variable name `TIMESTAMP3_FORMAT_WITH_TIME_ZONE` is a little confusing. Can we use `ISO_TIME_FORMAT` directly?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r431061963
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +174,13 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ return literal.getValue2();
}
Review comment:
For `Integral` and `string` it is correct, but i'm not sure if it is for other data types literal, such as `TIMESTAMP(3)`, can you make sure that ?
##########
File path: cassandra/src/test/java/org/apache/calcite/test/CassandraAdapterDataTypesTest.java
##########
@@ -79,6 +79,18 @@ static void load(Session session) {
+ ", f_varint INTEGER]");
}
+ @Test void testSimpleQuery() {
+ CalciteAssert.that()
Review comment:
`testSimpleQuery()` -> `testFilterWithNonStringLiteral` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r442644527
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
`ISO_DATETIME_FORMAT` and `ISO_DATETIME_FRACTIONAL_SECOND_FORMAT`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r435800913
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +174,13 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ return literal.getValue2();
}
Review comment:
@DonnyZone Creating an umbrella issue to fix the adapter literal data types is a good idea, go ahead if you think that is necessary. @wenhuitang for this patch, let's just fix the type that has no precision, which does not bring in confusion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] neoremind commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r433155542
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +174,13 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ return literal.getValue2();
}
Review comment:
For problem 2. Even if C* adapter supports precision, in avatica, `DateTimeUtils` handles milliseconds timestamp as below and neglects the precision, so that the string returned losses precision.
```
/** Helper for CAST({timestamp} AS VARCHAR(n)). */
public static String unixTimestampToString(long timestamp) {
return unixTimestampToString(timestamp, 0);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r433066236
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +179,28 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ String dateTimeFormatISO = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ new SimpleDateFormat(dateTimeFormatISO, Locale.ROOT);
+ dateFormatter.setTimeZone(DateTimeUtils.UTC_ZONE);
Review comment:
> Are there any codes we can reuse in `DataTimeUtils` ?
I did not find totally suitable code from DateTimeUtils. But I find that creating a SimpleDateFormat with setting time zone is used in several places in Calcite. So I add utility methods for creating SimpleDateFormat to the DateTimeStringUtils.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] neoremind commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r433155542
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +174,13 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ return literal.getValue2();
}
Review comment:
For problem 2. Even if C* adapter supports precision, in avatica, `DateTimeUtils` handles milliseconds timestamp as below and neglects the precision, so that the string returned losses precision, that could make test cases failing.
```
/** Helper for CAST({timestamp} AS VARCHAR(n)). */
public static String unixTimestampToString(long timestamp) {
return unixTimestampToString(timestamp, 0);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r442085701
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
How about ISO_DATETIME_SECONDS_FORMAT_STRING and ISO_DATETIME_MILLISECONDS_FORMAT_STRING?
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +180,26 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ getDateFormatter(TIMESTAMP_FORMAT_STRING2);
+ return dateFormatter.format(literal.getValue2());
+ case DATE:
+ assert value instanceof DateString;
+ return value.toString();
+ default:
+ return literal.getValue3();
+ }
Review comment:
Comparing with getValue2, getValue3 add the processing for type DECIMAL, not much difference. And IMO, getValue3 covered more situations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r435115515
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +180,26 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ Comparable value = RexLiteral.value(literal);
+ switch (literal.getTypeName()) {
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ assert value instanceof TimestampString;
+ final SimpleDateFormat dateFormatter =
+ getDateFormatter(TIMESTAMP3_FORMAT_WITH_TIME_ZONE);
Review comment:
> The variable name `TIMESTAMP3_FORMAT_WITH_TIME_ZONE` is a little confusing. Can we use `ISO_TIME_FORMAT` directly?
ISO 8601 format has several kinds of formats.
DateTimeUtils#TIMESTAMP_FORMAT_STRING also follows iso date string format.
How about using TIMESTAMP_FORMAT_STRING with different labels? Such as TIMESTAMP_FORMAT_STRING1 and TIMESTAMP_FORMAT_STRING2.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r443493968
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
The format name have been updated. Thanks a lot for your time on this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r442101891
##########
File path: core/src/main/java/org/apache/calcite/util/DateTimeStringUtils.java
##########
@@ -25,6 +29,15 @@
private DateTimeStringUtils() {}
+ /** The SimpleDateFormat string for ISO timestamps, "yyyy-MM-dd'T'HH:mm:ss'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING1 =
+ "yyyy-MM-dd'T'HH:mm:ss'Z'";
+
+ /** The SimpleDateFormat string for ISO timestamps with precisions, "yyyy-MM-dd'T'HH:mm:ss
+ * .SSS'Z'"*/
+ public static final String TIMESTAMP_FORMAT_STRING2 =
+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Review comment:
How about ISO_DATETIME_FORMAT and ISO_DATETIME_DECIMAL_FRACTION_FORMAT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 closed pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
danny0405 closed pull request #1990:
URL: https://github.com/apache/calcite/pull/1990
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on pull request #1990: [CALCITE-4026] CassandraFilter has generated wrong condition expression for filter with non string literal
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#issuecomment-643998846
@danny0405 @DonnyZone
Do you have time to see if this PR still needs improvement?
As for Time type used in CassandraFilter, I'd like to open a new JIRA.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wenhuitang commented on a change in pull request #1990: [CALCITE-4026] When the cassandra primary key is of type bigint,the query will report an error
Posted by GitBox <gi...@apache.org>.
wenhuitang commented on a change in pull request #1990:
URL: https://github.com/apache/calcite/pull/1990#discussion_r431575566
##########
File path: cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java
##########
@@ -174,14 +174,13 @@ private String translateMatch(RexNode condition) {
}
}
- /** Convert the value of a literal to a string.
+ /** Returns the value of the literal.
*
* @param literal Literal to translate
- * @return String representation of the literal
+ * @return The value of the literal in the form of the actual type.
*/
- private static String literalValue(RexLiteral literal) {
- Object value = literal.getValue2();
- return String.valueOf(value);
+ private static Object literalValue(RexLiteral literal) {
+ return literal.getValue2();
}
Review comment:
> For `Integral` and `string` it is correct, but i'm not sure if it is for other data types literal, such as `TIMESTAMP(3)`, can you make sure that ?
Thanks a lot for reviewing. This PR has been updated.
I have tested for type like Date, Timestamp and Time.
1.When the type of primary key 'f_time' is Time, it will throw exception when the filter condition is "f_time = Time '13:30:54.234'", because the type of 'f_time' is Bigint.
It seems another issue.
2.As for Timestamp(3), CassandraSchema create a Sql type Timestamp with precision 0. So It is possible to lose precision when get timestamp data from Cassandra.
As for above two issues, Whether we should solve them at this PR or creat new jira issues for them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org