You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/04/29 14:15:24 UTC

[GitHub] [arrow] wesm opened a new pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

wesm opened a new pull request #7060:
URL: https://github.com/apache/arrow/pull/7060


   This enables us to eliminate "special" handling of `Type::INTERVAL` since these types have a different internal data representation. The deleted code in this PR is evidence of this. 
   
   This is a breaking API change but since this is a lesser-used part of the project I don't think that many users will be impacted. 


----------------------------------------------------------------
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] [arrow] emkornfield commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-621628276


   In principal this looks ok to me.  It seems like there is at least one place that was missed /arrow/cpp/src/gandiva/jni/expression_registry_helper.cc:141:10: error: ‘INTERVAL’ is not a member of ‘arrow::Type::type’
   
   As long as the flatbuffers doesn't change, I'm OK with this.  Might be worth sending a mail out to dev@ just to make people aware of it?


----------------------------------------------------------------
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] [arrow] github-actions[bot] commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-621239897


   https://issues.apache.org/jira/browse/ARROW-8619


----------------------------------------------------------------
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] [arrow] emkornfield commented on a change in pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#discussion_r417769294



##########
File path: r/R/enums.R
##########
@@ -42,12 +42,35 @@ DateUnit <- enum("DateUnit", DAY = 0L, MILLI = 1L)
 #' @rdname enums
 #' @export
 Type <- enum("Type::type",
-  "NA" = 0L, BOOL = 1L, UINT8 = 2L, INT8 = 3L, UINT16 = 4L, INT16 = 5L,
-  UINT32 = 6L, INT32 = 7L, UINT64 = 8L, INT64 = 9L,
-  HALF_FLOAT = 10L, FLOAT = 11L, DOUBLE = 12L, STRING = 13L,
-  BINARY = 14L, FIXED_SIZE_BINARY = 15L, DATE32 = 16L, DATE64 = 17L, TIMESTAMP = 18L,
-  TIME32 = 19L, TIME64 = 20L, INTERVAL = 21L, DECIMAL = 22L, LIST = 23L, STRUCT = 24L,
-  UNION = 25L, DICTIONARY = 26L, MAP = 27L
+  "NA" = 0L,
+  BOOL = 1L,
+  UINT8 = 2L,
+  INT8 = 3L,
+  UINT16 = 4L,
+  INT16 = 5L,
+  UINT32 = 6L,
+  INT32 = 7L,
+  UINT64 = 8L,
+  INT64 = 9L,
+  HALF_FLOAT = 10L,
+  FLOAT = 11L,
+  DOUBLE = 12L,
+  STRING = 13L,
+  BINARY = 14L,
+  FIXED_SIZE_BINARY = 15L,
+  DATE32 = 16L,
+  DATE64 = 17L,
+  TIMESTAMP = 18L,
+  TIME32 = 19L,
+  TIME64 = 20L,
+  INTERVAL_MONTHS = 21L,

Review comment:
       are these enums stored anyplace?




----------------------------------------------------------------
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] [arrow] kszucs commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-622312875


   > The ursabot build failures are spurious
   
   Occasionally happens after a force push.


----------------------------------------------------------------
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] [arrow] wesm commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-621933237


   For some reason I can't get JNI running in my local setup
   
   ```
   CMake Error at /home/wesm/cpp-toolchain/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
     Could NOT find JNI (missing: JAVA_AWT_INCLUDE_PATH)
   Call Stack (most recent call first):
     /home/wesm/cpp-toolchain/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:393 (_FPHSA_FAILURE_MESSAGE)
     /home/wesm/cpp-toolchain/share/cmake-3.16/Modules/FindJNI.cmake:372 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
     src/gandiva/jni/CMakeLists.txt:27 (find_package)
   ```
   
   tried with 
   
   ```
   export JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64
   ```
   
   and 
   
   ```
   export JAVA_HOME=/usr/lib/jvm/java-11-openjdk-amd64
   ```
   
   It worked with
   
   ```
   export JAVA_HOME=/usr/lib/jvm/java-14-oracle
   ```
   
   though


----------------------------------------------------------------
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] [arrow] wesm commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-622156273


   The ursabot build failures are spurious


----------------------------------------------------------------
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] [arrow] wesm commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-622499382


   +1


----------------------------------------------------------------
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] [arrow] wesm commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-621872082


   Will fix the JNI issue and will notify the mailing list


----------------------------------------------------------------
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] [arrow] wesm commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-621472148


   Will fix the builds


----------------------------------------------------------------
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] [arrow] kszucs commented on pull request #7060: ARROW-8619: [C++] Use distinct enum values for MonthInterval, DayTimeInterval

Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7060:
URL: https://github.com/apache/arrow/pull/7060#issuecomment-622312610


   @ursabot build


----------------------------------------------------------------
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