You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/05/05 01:23:40 UTC
[GitHub] [drill] paul-rogers opened a new pull request #2076: DRILL-7729: Use java.time in column accessors
paul-rogers opened a new pull request #2076:
URL: https://github.com/apache/drill/pull/2076
# [DRILL-7729](https://issues.apache.org/jira/browse/DRILL-7729): Use java.time in column accessors
(Please replace `PR Title` with actual PR Title)
## Description
Uses` java.time` classes in the column accessors. Leaves Joda time for `Interval`, as it has no `java.time` equivalent.
This change allows the column accessors to work with Drill's JSON writer implementation. This PR includes a new `JsonWriter` based on a `RowSetReader`.
## Documentation
The change is transparent to users **except** in one particular case: a use of the "provided schema" feature in which the user has provided a format for a `DATE`, `TIME` or `TIMESTAMP` column. The `java.time` formats are slightly different than their Joda predecessors.
## Testing
Modified all tests which used Joda formats. Reran the full unit test suite.
----------------------------------------------------------------
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] [drill] cgivre commented on a change in pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2076:
URL: https://github.com/apache/drill/pull/2076#discussion_r421900398
##########
File path: exec/vector/src/main/java/org/apache/drill/exec/vector/DateUtilities.java
##########
@@ -195,7 +197,27 @@ public static int timeToMillis(int hours, int minutes, int seconds, int millis)
* @param localTime Java local time
* @return Drill form of the time
Review comment:
@paul-rogers
Would you mind please adding a brief javadoc to this class explaining what a Drill form of the time is?
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/scan/convert/StandardConversions.java
##########
@@ -166,6 +171,12 @@ public static DirectConverter newInstance(
try {
final Constructor<? extends DirectConverter> ctor = conversionClass.getDeclaredConstructor(ScalarWriter.class, Map.class);
return ctor.newInstance(baseWriter, properties);
+ } catch (final InvocationTargetException e) {
+ throw UserException.validationError(e.getTargetException())
+ .addContext("Converter setup failed")
+ .addContext("Conversion class" + conversionClass.getName())
+ // .addContext(errorContext) // Add after merge
Review comment:
Should we add this line back in?
----------------------------------------------------------------
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] [drill] vvysotskyi commented on pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2076:
URL: https://github.com/apache/drill/pull/2076#issuecomment-633133080
@paul-rogers, did you have a chance to take a look at @cgivre comments?
----------------------------------------------------------------
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] [drill] paul-rogers commented on pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on pull request #2076:
URL: https://github.com/apache/drill/pull/2076#issuecomment-767847071
@luocooong, thanks for the reminder. Looks like I originally factored out the Java time stuff for its own review. Got no takers, it seems, and I forgot about it. So, when @cgivre asked me to finish the streaming JSON stuff, I just included the time stuff as well.
If the team prefers, I can pull the Java time stuff out of the streaming JSON PR and replace the code here with the updated version. Or, if the team is OK with the single big PR, we can keep it in the streaming JSON PR.
How would you like to proceed?
----------------------------------------------------------------
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] [drill] paul-rogers closed pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
paul-rogers closed pull request #2076:
URL: https://github.com/apache/drill/pull/2076
----------------------------------------------------------------
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] [drill] luocooong commented on pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2076:
URL: https://github.com/apache/drill/pull/2076#issuecomment-767613143
@paul-rogers Hello, Is this PR still necessary to merge? Is it replaced by [#2149](https://github.com/apache/drill/pull/2149)?
----------------------------------------------------------------
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] [drill] paul-rogers commented on a change in pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2076:
URL: https://github.com/apache/drill/pull/2076#discussion_r429685226
##########
File path: exec/vector/src/main/java/org/apache/drill/exec/vector/DateUtilities.java
##########
@@ -195,7 +197,27 @@ public static int timeToMillis(int hours, int minutes, int seconds, int millis)
* @param localTime Java local time
* @return Drill form of the time
Review comment:
Turns out the format is explained two lines above, but repeated it in the `@return` as requested.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/scan/convert/StandardConversions.java
##########
@@ -166,6 +171,12 @@ public static DirectConverter newInstance(
try {
final Constructor<? extends DirectConverter> ctor = conversionClass.getDeclaredConstructor(ScalarWriter.class, Map.class);
return ctor.newInstance(baseWriter, properties);
+ } catch (final InvocationTargetException e) {
+ throw UserException.validationError(e.getTargetException())
+ .addContext("Converter setup failed")
+ .addContext("Conversion class" + conversionClass.getName())
+ // .addContext(errorContext) // Add after merge
Review comment:
As the comment (tersely) suggests, the error context can be added after the merge of a different PR which provides the error context. This comment will show up in a merge conflict and remind me to make that change.
----------------------------------------------------------------
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] [drill] vvysotskyi commented on pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2076:
URL: https://github.com/apache/drill/pull/2076#issuecomment-767849492
I propose to leave changes in bigger PR, to avoid additional work on splitting it and rebasing this one onto the master.
----------------------------------------------------------------
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] [drill] paul-rogers commented on pull request #2076: DRILL-7729: Use java.time in column accessors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on pull request #2076:
URL: https://github.com/apache/drill/pull/2076#issuecomment-767860717
@vvysotskyi, thanks for the suggestion. Given that, I'll go ahead and close 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