[GitHub] [arrow] github-actions[bot] commented on pull request #8597: ARROW-10492: [Java][JDBC] Allow users to config the mapping between SQL types and Arrow types
github-actions[bot] commented on pull request #8597: URL: https://github.com/apache/arrow/pull/8597#issuecomment-722193375 https://issues.apache.org/jira/browse/ARROW-10492 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: us...@infra.apache.org
[GitHub] [arrow] liyafan82 opened a new pull request #8597: ARROW-10492: [Java][JDBC] Allow users to config the mapping between SQL types and Arrow types
liyafan82 opened a new pull request #8597: URL: https://github.com/apache/arrow/pull/8597 According to the current implementation of JDBC adapter, the conversion between SQL types and Arrow types is hard-coded. This will cause some problems in practice: 1. The appropriate conversion may vary for different databases. For example, for SQL Server, type real corresponds to 4 byte floating point values (https://docs.microsoft.com/en-us/sql/t-sql/data-types/float-and-real-transact-sql?view=sql-server-ver15), whereas for SQLite, real corresponds to 8 byte floating point values (https://www.sqlitetutorial.net/sqlite-data-types/). If the maping is not right, some extra conversion would happen, which can impact performance severely. 2. Our current implementation determines the type conversion solely based on the type ID. However, the appropriate conversion may also depend some other information, like precision and scale. For example, for FLOAT( n ), it should correspond to 4 byte floating point values, if n <= 24, otherwise, it should correspond to 8 byte floating point values. To address the problems, we should allow users to customize the conversion between SQL and Arrow types. 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8596: ARROW-10412: [C++] Improve grpc_cpp_plugin detection
github-actions[bot] commented on pull request #8596: URL: https://github.com/apache/arrow/pull/8596#issuecomment-722185791 Revision: 39b6a98c4f460c3061c5b8c89c3b1eed9083b930 Submitted crossbow builds: [ursa-labs/crossbow @ actions-692](https://github.com/ursa-labs/crossbow/branches/all?query=actions-692) |Task|Status| ||--| |centos-6-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-692-github-centos-6-amd64)| |centos-7-aarch64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |centos-7-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-692-github-centos-7-amd64)| |centos-8-aarch64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |centos-8-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-692-github-centos-8-amd64)| |conda-clean|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-clean)| |conda-linux-gcc-py36-aarch64|[](https://cloud.drone.io/ursa-labs/crossbow)| |conda-linux-gcc-py36-cpu|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-linux-gcc-py36-cpu)| |conda-linux-gcc-py36-cuda|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-linux-gcc-py36-cuda)| |conda-linux-gcc-py37-aarch64|[](https://cloud.drone.io/ursa-labs/crossbow)| |conda-linux-gcc-py37-cpu|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-linux-gcc-py37-cpu)| |conda-linux-gcc-py37-cuda|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-linux-gcc-py37-cuda)| |conda-linux-gcc-py38-aarch64|[](https://cloud.drone.io/ursa-labs/crossbow)| |conda-linux-gcc-py38-cpu|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-linux-gcc-py38-cpu)| |conda-linux-gcc-py38-cuda|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-linux-gcc-py38-cuda)| |conda-osx-clang-py36|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-osx-clang-py36)| |conda-osx-clang-py37|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-osx-clang-py37)| |conda-osx-clang-py38|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-692-azure-conda-osx-clang-py38)|
[GitHub] [arrow] kou commented on pull request #8596: ARROW-10412: [C++] Improve grpc_cpp_plugin detection
kou commented on pull request #8596: URL: https://github.com/apache/arrow/pull/8596#issuecomment-722183440 @github-actions crossbow submit -g nightly 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: us...@infra.apache.org
[GitHub] [arrow] malthe edited a comment on pull request #8588: ARROW-4637: [Python] Must lock on import, to avoid deadlock due to circular imports
malthe edited a comment on pull request #8588: URL: https://github.com/apache/arrow/pull/8588#issuecomment-722174670 @wesm due to circular imports – in Pandas. Normally, this is not a problem because at least they're resolved in a predictable way, but it can cause deadlocks when multiple threads are trying to do it concurrently. 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: us...@infra.apache.org
[GitHub] [arrow] malthe commented on pull request #8588: ARROW-4637: [Python] Must lock on import, to avoid deadlock due to circular imports
malthe commented on pull request #8588: URL: https://github.com/apache/arrow/pull/8588#issuecomment-722174670 @wesm due to circular imports. Normally, this is not a problem because at least they're resolved in a predictable way, but it can cause deadlocks when multiple threads are trying to do it concurrently. 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8596: ARROW-10412: [C++] Improve grpc_cpp_plugin detection
github-actions[bot] commented on pull request #8596: URL: https://github.com/apache/arrow/pull/8596#issuecomment-722173389 https://issues.apache.org/jira/browse/ARROW-10412 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: us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
kylepbit commented on pull request #8572: URL: https://github.com/apache/arrow/pull/8572#issuecomment-722171554 Regarding the comment about sessions and cookies - yes, there is definite interest in making sure that the work there is building on top of, instead of parallel to, one another. 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: us...@infra.apache.org
[GitHub] [arrow] kou opened a new pull request #8596: ARROW-10412: [C++] Improve grpc_cpp_plugin detection
kou opened a new pull request #8596: URL: https://github.com/apache/arrow/pull/8596 If gRPC::grpc_cpp_plugin doesn't use IMPORTED_LOCATION nor IMPORTED_LOCATION_RELEASE such as IMPORTED_LOCATION_RELWITHDEBINFO, grpc_cpp_plugin wasn't found. 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: us...@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #8056: ARROW-9861: [Java] Support big-endian in DecimalVector
kiszk commented on pull request #8056: URL: https://github.com/apache/arrow/pull/8056#issuecomment-722146327 @BryanCutler Thank you. One comment. According to [the document](https://github.com/apache/arrow/blob/master/docs/source/developers/contributing.rst#endianness), benchmarking is necessary before merging features (I think that the test code does not affect the performance) > Benchmarks for performance critical parts of the code to demonstrate no regression. I am working for benchmark bot for Java [here](https://github.com/apache/arrow/pull/8210). It would be good to merge new features after this bot will be available. cc @emkornfield 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: us...@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8592: ARROW-10495: [Packaging][deb] Move FindRE2.cmake to libarrow-dev
kou commented on pull request #8592: URL: https://github.com/apache/arrow/pull/8592#issuecomment-722124689 +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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8592: ARROW-10495: [Packaging][deb] Move FindRE2.cmake to libarrow-dev
github-actions[bot] commented on pull request #8592: URL: https://github.com/apache/arrow/pull/8592#issuecomment-722059856 Revision: 142e57ca9753af3f7770b688169cc02ee3e21c66 Submitted crossbow builds: [ursa-labs/crossbow @ actions-691](https://github.com/ursa-labs/crossbow/branches/all?query=actions-691) |Task|Status| ||--| |debian-buster-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-691-github-debian-buster-amd64)| |debian-buster-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |debian-stretch-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-691-github-debian-stretch-amd64)| |debian-stretch-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |ubuntu-bionic-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-691-github-ubuntu-bionic-amd64)| |ubuntu-bionic-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |ubuntu-focal-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-691-github-ubuntu-focal-amd64)| |ubuntu-focal-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |ubuntu-xenial-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-691-github-ubuntu-xenial-amd64)| |ubuntu-xenial-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| 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: us...@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8592: ARROW-10495: [Packaging][deb] Move FindRE2.cmake to libarrow-dev
kou commented on pull request #8592: URL: https://github.com/apache/arrow/pull/8592#issuecomment-722059230 @github-actions crossbow submit debian-* ubuntu-* 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: us...@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #8588: ARROW-4637: [Python] Must lock on import, to avoid deadlock due to circular imports
wesm commented on pull request #8588: URL: https://github.com/apache/arrow/pull/8588#issuecomment-722058751 Do you know why the deadlock occurs? It's not especially intuitive to me 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8595: ARROW-10499: [C++][Java] Fix ORC Java JNI Crash
github-actions[bot] commented on pull request #8595: URL: https://github.com/apache/arrow/pull/8595#issuecomment-722056096 https://issues.apache.org/jira/browse/ARROW-10499 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: us...@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8386: ARROW-10224: [Python] Build, test, and support Python 3.9
kou commented on pull request #8386: URL: https://github.com/apache/arrow/pull/8386#issuecomment-722052960 I see. It seems that our Crossbow + AppVeyor usage isn't suitable for AppVeyor. I will port the Crossbow tasks to GitHub Actions from AppVeyor. 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: us...@infra.apache.org
[GitHub] [arrow] terencehonles opened a new pull request #8595: ARROW-10499: [C++][Java] Fix ORC Java JNI Crash
terencehonles opened a new pull request #8595: URL: https://github.com/apache/arrow/pull/8595 `OrcStripeReaderJniWrapper::getSchema` previously used `nullptr` for the memory pool parameter to `arrow::ipc::SerializeSchema` to implicitly call `arrow::default_memory_pool()`. As part of https://issues.apache.org/jira/browse/ARROW-10080, a check was placed to fail on `nullptr` being provided. This change removes the check, but also explicitly calls `arrow::default_memory_pool()` which is used elsewhere in the JNI wrapper. 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: us...@infra.apache.org
[GitHub] [arrow] terencehonles commented on pull request #8533: ARROW-10080: [R] Call gc() and try again in MemoryPool
terencehonles commented on pull request #8533: URL: https://github.com/apache/arrow/pull/8533#issuecomment-722045397 It looks like the offending line is https://github.com/apache/arrow/blob/master/cpp/src/jni/orc/jni_wrapper.cpp#L229 and I'm updating that to not pass null, but since passing a null pointer was previously supported I'm going to also remove the check in case it's used elsewhere. 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: us...@infra.apache.org
[GitHub] [arrow] Bei-z commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
Bei-z commented on a change in pull request #8542: URL: https://github.com/apache/arrow/pull/8542#discussion_r517702346 ## File path: cpp/src/arrow/util/basic_decimal.cc ## @@ -451,6 +463,27 @@ static int64_t FillInArray(const BasicDecimal128& value, uint32_t* array, return 1; } +/// Expands the given value into an array of ints so that we can work on Review comment: Done. Thank you! 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: us...@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
lidavidm closed pull request #8586: URL: https://github.com/apache/arrow/pull/8586 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: us...@infra.apache.org
[GitHub] [arrow] Bei-z commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
Bei-z commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r51777
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -395,6 +395,31 @@ BasicDecimal128& BasicDecimal128::operator*=(const
BasicDecimal128& right) {
return *this;
}
+/// Expands the given big endian array of uint64_t into an array of uint32_t.
+/// The value of input array is expected to be positive. The result_array will
Review comment:
Done. Thank you!
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -451,6 +463,27 @@ static int64_t FillInArray(const BasicDecimal128& value,
uint32_t* array,
return 1;
}
+/// Expands the given value into an array of ints so that we can work on
+/// it. The array will be converted to an absolute value and the wasNegative
Review comment:
Done. Thank you!
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:
us...@infra.apache.org
[GitHub] [arrow] Bei-z commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
Bei-z commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r517699770
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -404,23 +429,10 @@ BasicDecimal128& BasicDecimal128::operator*=(const
BasicDecimal128& right) {
/// \result the output length of the array
static int64_t FillInArray(const BasicDecimal128& value, uint32_t* array,
bool& was_negative) {
- uint64_t high;
- uint64_t low;
- const int64_t highbits = value.high_bits();
- const uint64_t lowbits = value.low_bits();
-
- if (highbits < 0) {
-low = ~lowbits + 1;
-high = static_cast(~highbits);
-if (low == 0) {
- ++high;
-}
-was_negative = true;
- } else {
-low = lowbits;
-high = static_cast(highbits);
-was_negative = false;
- }
+ BasicDecimal128 abs_value = BasicDecimal128::Abs(value);
+ was_negative = value.high_bits() < 0;
+ uint64_t high = static_cast(abs_value.high_bits());
Review comment:
Comment added below. Thanks!
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:
us...@infra.apache.org
[GitHub] [arrow] Bei-z commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
Bei-z commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r517699593
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -404,51 +429,33 @@ BasicDecimal128& BasicDecimal128::operator*=(const
BasicDecimal128& right) {
/// \result the output length of the array
static int64_t FillInArray(const BasicDecimal128& value, uint32_t* array,
bool& was_negative) {
- uint64_t high;
- uint64_t low;
- const int64_t highbits = value.high_bits();
- const uint64_t lowbits = value.low_bits();
-
- if (highbits < 0) {
-low = ~lowbits + 1;
-high = static_cast(~highbits);
-if (low == 0) {
- ++high;
-}
-was_negative = true;
- } else {
-low = lowbits;
-high = static_cast(highbits);
-was_negative = false;
- }
-
- if (high != 0) {
-if (high > std::numeric_limits::max()) {
- array[0] = static_cast(high >> 32);
- array[1] = static_cast(high);
- array[2] = static_cast(low >> 32);
- array[3] = static_cast(low);
- return 4;
-}
-
-array[0] = static_cast(high);
-array[1] = static_cast(low >> 32);
-array[2] = static_cast(low);
-return 3;
- }
-
- if (low >= std::numeric_limits::max()) {
-array[0] = static_cast(low >> 32);
-array[1] = static_cast(low);
-return 2;
- }
+ BasicDecimal128 abs_value = BasicDecimal128::Abs(value);
+ was_negative = value.high_bits() < 0;
+ std::array abs_big_endian_array = {
+ static_cast(abs_value.high_bits()),
+ static_cast(abs_value.low_bits())};
+ return FillInArray(abs_big_endian_array, array);
+}
- if (low == 0) {
-return 0;
+/// Expands the given value into an array of ints so that we can work on
+/// it. The array will be converted to an absolute value and the wasNegative
+/// flag will be set appropriately. The array will remove leading zeros from
+/// the value.
+/// \param array an array of length 8 to set with the value
+/// \param was_negative a flag for whether the value was original negative
+/// \result the output length of the array
+static int64_t FillInArray(const BasicDecimal256& value, uint32_t* array,
+ bool& was_negative) {
+ BasicDecimal256 positive_value = value;
+ was_negative = false;
+ int64_t highest_bit = positive_value.little_endian_array()[3];
+ if (highest_bit < 0) {
+positive_value.Negate();
+was_negative = true;
}
-
- array[0] = static_cast(low);
- return 1;
+ std::array value_big_endian_array =
positive_value.little_endian_array();
+ std::reverse(value_big_endian_array.begin(), value_big_endian_array.end());
Review comment:
Updated accordingly. Thank you!
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:
us...@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
lidavidm commented on pull request #8586: URL: https://github.com/apache/arrow/pull/8586#issuecomment-722028813 Thanks, will merge on green (ignoring the JNI test failure) 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: us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
jduo commented on a change in pull request #8586:
URL: https://github.com/apache/arrow/pull/8586#discussion_r517690070
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/RequestContextAdapter.java
##
@@ -30,7 +30,7 @@
*/
public class RequestContextAdapter implements RequestContext {
public static final Context.Key REQUEST_CONTEXT_KEY =
- Context.key("arrow-flight-request-context");
+ Context.keyWithDefault("arrow-flight-request-context", new
RequestContextAdapter());
Review comment:
Done
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:
us...@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
lidavidm commented on a change in pull request #8572:
URL: https://github.com/apache/arrow/pull/8572#discussion_r517682878
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import java.util.function.Consumer;
+
+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer {
Review comment:
Yes - within the body of an RPC handler, how do I access the headers
that the client sent for that call?
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:
us...@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
lidavidm commented on pull request #8572: URL: https://github.com/apache/arrow/pull/8572#issuecomment-722021593 Just a higher level comment - I see on ARROW-10428 cookie support is being worked on and ARROW-10427 is about session headers. Since cookies are a particular header and sessions are (usually) a particular cookie, is there any interest in making sure all these features work together and are built on top of each other? 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: us...@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
lidavidm commented on a change in pull request #8586:
URL: https://github.com/apache/arrow/pull/8586#discussion_r517681068
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/RequestContextAdapter.java
##
@@ -30,7 +30,7 @@
*/
public class RequestContextAdapter implements RequestContext {
public static final Context.Key REQUEST_CONTEXT_KEY =
- Context.key("arrow-flight-request-context");
+ Context.keyWithDefault("arrow-flight-request-context", new
RequestContextAdapter());
Review comment:
Sorry - now that I think about this again, this is shared mutable state,
so wouldn't it be possible (though unlikely) for different calls to persist
data into this instance of RequestContextAdapter? It might be better to leave
this nullable and check usages of it instead.
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:
us...@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8581: ARROW-10441: [Java] Prevent closure of shared channels for FlightClient
lidavidm commented on pull request #8581: URL: https://github.com/apache/arrow/pull/8581#issuecomment-722019046 No problem, I was just waiting for other builds to pass. Thanks! 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: us...@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #8581: ARROW-10441: [Java] Prevent closure of shared channels for FlightClient
lidavidm closed pull request #8581: URL: https://github.com/apache/arrow/pull/8581 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: us...@infra.apache.org
[GitHub] [arrow] MingyuZhong commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
MingyuZhong commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r517062023
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -404,51 +429,33 @@ BasicDecimal128& BasicDecimal128::operator*=(const
BasicDecimal128& right) {
/// \result the output length of the array
static int64_t FillInArray(const BasicDecimal128& value, uint32_t* array,
bool& was_negative) {
- uint64_t high;
- uint64_t low;
- const int64_t highbits = value.high_bits();
- const uint64_t lowbits = value.low_bits();
-
- if (highbits < 0) {
-low = ~lowbits + 1;
-high = static_cast(~highbits);
-if (low == 0) {
- ++high;
-}
-was_negative = true;
- } else {
-low = lowbits;
-high = static_cast(highbits);
-was_negative = false;
- }
-
- if (high != 0) {
-if (high > std::numeric_limits::max()) {
- array[0] = static_cast(high >> 32);
- array[1] = static_cast(high);
- array[2] = static_cast(low >> 32);
- array[3] = static_cast(low);
- return 4;
-}
-
-array[0] = static_cast(high);
-array[1] = static_cast(low >> 32);
-array[2] = static_cast(low);
-return 3;
- }
-
- if (low >= std::numeric_limits::max()) {
-array[0] = static_cast(low >> 32);
-array[1] = static_cast(low);
-return 2;
- }
+ BasicDecimal128 abs_value = BasicDecimal128::Abs(value);
+ was_negative = value.high_bits() < 0;
+ std::array abs_big_endian_array = {
+ static_cast(abs_value.high_bits()),
+ static_cast(abs_value.low_bits())};
+ return FillInArray(abs_big_endian_array, array);
+}
- if (low == 0) {
-return 0;
+/// Expands the given value into an array of ints so that we can work on
+/// it. The array will be converted to an absolute value and the wasNegative
+/// flag will be set appropriately. The array will remove leading zeros from
+/// the value.
+/// \param array an array of length 8 to set with the value
+/// \param was_negative a flag for whether the value was original negative
+/// \result the output length of the array
+static int64_t FillInArray(const BasicDecimal256& value, uint32_t* array,
+ bool& was_negative) {
+ BasicDecimal256 positive_value = value;
+ was_negative = false;
+ int64_t highest_bit = positive_value.little_endian_array()[3];
+ if (highest_bit < 0) {
+positive_value.Negate();
+was_negative = true;
}
-
- array[0] = static_cast(low);
- return 1;
+ std::array value_big_endian_array =
positive_value.little_endian_array();
+ std::reverse(value_big_endian_array.begin(), value_big_endian_array.end());
Review comment:
Can FillInArray below take little-endian array so you don't need to call
std::reverse?
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -451,6 +463,27 @@ static int64_t FillInArray(const BasicDecimal128& value,
uint32_t* array,
return 1;
}
+/// Expands the given value into an array of ints so that we can work on
Review comment:
Comment that the output array is in big-endian.
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -451,6 +463,27 @@ static int64_t FillInArray(const BasicDecimal128& value,
uint32_t* array,
return 1;
}
+/// Expands the given value into an array of ints so that we can work on
+/// it. The array will be converted to an absolute value and the wasNegative
Review comment:
was_negative
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -404,23 +429,10 @@ BasicDecimal128& BasicDecimal128::operator*=(const
BasicDecimal128& right) {
/// \result the output length of the array
static int64_t FillInArray(const BasicDecimal128& value, uint32_t* array,
bool& was_negative) {
- uint64_t high;
- uint64_t low;
- const int64_t highbits = value.high_bits();
- const uint64_t lowbits = value.low_bits();
-
- if (highbits < 0) {
-low = ~lowbits + 1;
-high = static_cast(~highbits);
-if (low == 0) {
- ++high;
-}
-was_negative = true;
- } else {
-low = lowbits;
-high = static_cast(highbits);
-was_negative = false;
- }
+ BasicDecimal128 abs_value = BasicDecimal128::Abs(value);
+ was_negative = value.high_bits() < 0;
+ uint64_t high = static_cast(abs_value.high_bits());
Review comment:
Please add a comment about why FillInArray is not called.
##
File path: cpp/src/arrow/util/basic_decimal.cc
##
@@ -395,6 +395,31 @@ BasicDecimal128& BasicDecimal128::operator*=(const
BasicDecimal128& right) {
return *this;
}
+/// Expands the given big endian array of uint64_t into an array of uint32_t.
+/// The value of input array is expected to be positive. The result_array will
Review comment:
positive -> non-negative?
This is an automated message from the Apache Git Service.
To respond to the message,
[GitHub] [arrow] kszucs commented on pull request #8386: ARROW-10224: [Python] Build, test, and support Python 3.9
kszucs commented on pull request #8386: URL: https://github.com/apache/arrow/pull/8386#issuecomment-722014794 @kou crossbow has been deleted from appveyor and I'm banned to do anything. Support has not responded yet. You may try to setup the appveyor integration for that repository, perhaps it would work for your user. Otherwise we can switch to another crossbow repository or port the windows wheel builds to another CI. 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: us...@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #8593: ARROW-10371: [R] Linux system requirements check needs to support older cmake versions
nealrichardson closed pull request #8593: URL: https://github.com/apache/arrow/pull/8593 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: us...@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #8594: ARROW-10496: [R][CI] Fix conda-r job
nealrichardson closed pull request #8594: URL: https://github.com/apache/arrow/pull/8594 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: us...@infra.apache.org
[GitHub] [arrow] tifflhl closed pull request #8583: Arrow 10427: [Flight][Java] Add optional session header
tifflhl closed pull request #8583: URL: https://github.com/apache/arrow/pull/8583 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8594: ARROW-10496: [R][CI] Fix conda-r job
github-actions[bot] commented on pull request #8594: URL: https://github.com/apache/arrow/pull/8594#issuecomment-722000264 https://issues.apache.org/jira/browse/ARROW-10496 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: us...@infra.apache.org
[GitHub] [arrow] carols10cents commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
carols10cents commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721995452 Additional data to consider: I'm not confident the implementations are exactly comparable, but the C++ Buffer type does not take its [`capacity`](https://github.com/apache/arrow/blob/1c223f517f30f4d577a00eae3d0dbca929b2ffac/cpp/src/arrow/buffer.h#L64) into account [in its `Equals` implementations](https://github.com/apache/arrow/blob/1c223f517f30f4d577a00eae3d0dbca929b2ffac/cpp/src/arrow/buffer.cc#L91-L101). 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8594: ARROW-10496: [R][CI] Fix conda-r job
github-actions[bot] commented on pull request #8594: URL: https://github.com/apache/arrow/pull/8594#issuecomment-721994447 Revision: 52870b5d5dd994e34c0f2f0a7ef62dea39d2b303 Submitted crossbow builds: [ursa-labs/crossbow @ actions-690](https://github.com/ursa-labs/crossbow/branches/all?query=actions-690) |Task|Status| ||--| |test-conda-r-4.0|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-690-github-test-conda-r-4.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: us...@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8594: ARROW-10496: [R][CI] Fix conda-r job
nealrichardson commented on pull request #8594: URL: https://github.com/apache/arrow/pull/8594#issuecomment-721993677 @github-actions crossbow submit test-conda-r-4.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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8593: ARROW-10371: [R] Linux system requirements check needs to support older cmake versions
github-actions[bot] commented on pull request #8593: URL: https://github.com/apache/arrow/pull/8593#issuecomment-721991686 Revision: 96363324ff585f5068aa0b7515c10968f645c641 Submitted crossbow builds: [ursa-labs/crossbow @ actions-689](https://github.com/ursa-labs/crossbow/branches/all?query=actions-689) |Task|Status| ||--| |homebrew-r-autobrew|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |test-conda-r-4.0|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-689-github-test-conda-r-4.0)| |test-r-linux-as-cran|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-689-github-test-r-linux-as-cran)| |test-r-rhub-ubuntu-gcc-release|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rhub-ubuntu-gcc-release)| |test-r-rocker-r-base-latest|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rocker-r-base-latest)| |test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rstudio-r-base-3.6-bionic)| |test-r-rstudio-r-base-3.6-centos6|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rstudio-r-base-3.6-centos6)| |test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rstudio-r-base-3.6-centos8)| |test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rstudio-r-base-3.6-opensuse15)| |test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-r-rstudio-r-base-3.6-opensuse42)| |test-ubuntu-18.04-r-sanitizer|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1=actions-689-azure-test-ubuntu-18.04-r-sanitizer)| 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: us...@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8593: ARROW-10371: [R] Linux system requirements check needs to support older cmake versions
nealrichardson commented on pull request #8593: URL: https://github.com/apache/arrow/pull/8593#issuecomment-721990977 @github-actions crossbow submit -g r 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8593: ARROW-10371: [R] Linux system requirements check needs to support older cmake versions
github-actions[bot] commented on pull request #8593: URL: https://github.com/apache/arrow/pull/8593#issuecomment-721986290 https://issues.apache.org/jira/browse/ARROW-10371 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8592: ARROW-10495: [Packaging][deb] Move FindRE2.cmake to libarrow-dev
github-actions[bot] commented on pull request #8592: URL: https://github.com/apache/arrow/pull/8592#issuecomment-721980494 Revision: e188dd5aba58c10970d8531748a873cd9605d475 Submitted crossbow builds: [ursa-labs/crossbow @ actions-688](https://github.com/ursa-labs/crossbow/branches/all?query=actions-688) |Task|Status| ||--| |debian-buster-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-688-github-debian-buster-amd64)| |debian-buster-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |debian-stretch-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-688-github-debian-stretch-amd64)| |debian-stretch-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |ubuntu-bionic-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-688-github-ubuntu-bionic-amd64)| |ubuntu-bionic-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |ubuntu-focal-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-688-github-ubuntu-focal-amd64)| |ubuntu-focal-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| |ubuntu-xenial-amd64|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-688-github-ubuntu-xenial-amd64)| |ubuntu-xenial-arm64|[](https://travis-ci.org/ursa-labs/crossbow/branches)| 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: us...@infra.apache.org
[GitHub] [arrow] kou opened a new pull request #8592: ARROW-10495: [Packaging][deb] Move FindRE2.cmake to libarrow-dev
kou opened a new pull request #8592: URL: https://github.com/apache/arrow/pull/8592 It's required for libarrow_bundled_dependencies.a in libarrow-dev. So we need to put FindRE2.cmake to libarrow-dev not libgandiva-dev. 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: us...@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8592: ARROW-10495: [Packaging][deb] Move FindRE2.cmake to libarrow-dev
kou commented on pull request #8592: URL: https://github.com/apache/arrow/pull/8592#issuecomment-721979886 @github-actions crossbow submit debian-* ubuntu-* 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: us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on a change in pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
kylepbit commented on a change in pull request #8572:
URL: https://github.com/apache/arrow/pull/8572#discussion_r517620350
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import java.util.function.Consumer;
+
+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer {
Review comment:
Not sure I understand the comment, are you simply saying you can't
correlate the headers that are incoming with the RPC call that generated them?
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import java.util.function.Consumer;
+
+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer {
Review comment:
Not sure I understand the comment, are you saying you can't correlate
the headers that are incoming with the RPC call that generated 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:
us...@infra.apache.org
[GitHub] [arrow] Bei-z commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
Bei-z commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r517611814
##
File path: cpp/src/arrow/util/decimal_benchmark.cc
##
@@ -158,6 +158,7 @@ static void BinaryMathOp256(benchmark::State& state) { //
NOLINT non-const refe
for (auto _ : state) {
for (int x = 0; x < kValueSize; x += 5) {
benchmark::DoNotOptimize(v1[x + 2] * v2[x + 2]);
+ benchmark::DoNotOptimize(v1[x + 3] / v2[x + 3]);
Review comment:
As discussed offline, moved back the if statements in FillInArray for
BasicDecimal128, the that the current benchmark result is:
BinaryMathOp128152 ns 152 ns 4284240
items_per_second=65.6334M/s
BinaryMathOp256163 ns 163 ns 4186744
items_per_second=61.4706M/s
Compared with the benchmark result without this pr:
BinaryMathOp128154 ns 154 ns 4423113
items_per_second=64.9309M/s
BinaryMathOp256 35.4 ns 35.4 ns 18341932
items_per_second=282.487M/s
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:
us...@infra.apache.org
[GitHub] [arrow] Bei-z commented on a change in pull request #8542: ARROW-10407: [C++] Add BasicDecimal256 Division Support
Bei-z commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r517611814
##
File path: cpp/src/arrow/util/decimal_benchmark.cc
##
@@ -158,6 +158,7 @@ static void BinaryMathOp256(benchmark::State& state) { //
NOLINT non-const refe
for (auto _ : state) {
for (int x = 0; x < kValueSize; x += 5) {
benchmark::DoNotOptimize(v1[x + 2] * v2[x + 2]);
+ benchmark::DoNotOptimize(v1[x + 3] / v2[x + 3]);
Review comment:
As discussed offline, moved back the if statement in FillInArray for
BasicDecimal128, the that the current benchmark result is:
BinaryMathOp128152 ns 152 ns 4284240
items_per_second=65.6334M/s
BinaryMathOp256163 ns 163 ns 4186744
items_per_second=61.4706M/s
Compared with the benchmark result without this pr:
BinaryMathOp128154 ns 154 ns 4423113
items_per_second=64.9309M/s
BinaryMathOp256 35.4 ns 35.4 ns 18341932
items_per_second=282.487M/s
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:
us...@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #8574: ARROW-10468: [C++][Compute] Provide KernelExecutor instead of FunctionExecutor
bkietz closed pull request #8574: URL: https://github.com/apache/arrow/pull/8574 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: us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on pull request #8581: ARROW-10441: [Java] Prevent closure of shared channels for FlightClient
kylepbit commented on pull request #8581: URL: https://github.com/apache/arrow/pull/8581#issuecomment-721949470 @lidavidm I believe the JNI issue is not related and is a common problem - how can we get this merged? 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: us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8554: ARROW-10428: [FlightRPC][Java] Add support for HTTP cookies
jduo commented on a change in pull request #8554:
URL: https://github.com/apache/arrow/pull/8554#discussion_r517601440
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java
##
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight.client;
+
+import java.net.HttpCookie;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.flight.CallHeaders;
+import org.apache.arrow.flight.CallInfo;
+import org.apache.arrow.flight.CallStatus;
+import org.apache.arrow.flight.FlightClientMiddleware;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * A client middleware for receiving and sending cookie information.
+ * Note that this class will not persist permanent cookies beyond the lifetime
+ * of this session.
+ */
+public class ClientCookieMiddleware implements FlightClientMiddleware {
+ private static final String SET_COOKIE_HEADER = "Set-Cookie";
+ private static final String SET_COOKIE2_HEADER = "Set-Cookie2";
+ private static final String COOKIE_HEADER = "Cookie";
+
+ // Use a map to track the most recent version of a cookie from the server.
+ // Note that cookie names are case-sensitive (but header names aren't).
+ private Map cookies = new HashMap<>();
Review comment:
Done.
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java
##
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight.client;
+
+import java.net.HttpCookie;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.flight.CallHeaders;
+import org.apache.arrow.flight.CallInfo;
+import org.apache.arrow.flight.CallStatus;
+import org.apache.arrow.flight.FlightClientMiddleware;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * A client middleware for receiving and sending cookie information.
+ * Note that this class will not persist permanent cookies beyond the lifetime
+ * of this session.
+ */
+public class ClientCookieMiddleware implements FlightClientMiddleware {
+ private static final String SET_COOKIE_HEADER = "Set-Cookie";
+ private static final String SET_COOKIE2_HEADER = "Set-Cookie2";
Review comment:
Removed Set-Cookie2 support.
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java
##
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing
[GitHub] [arrow] kou commented on pull request #8386: ARROW-10224: [Python] Build, test, and support Python 3.9
kou commented on pull request #8386: URL: https://github.com/apache/arrow/pull/8386#issuecomment-721944861 It seems that https://ci.appveyor.com/project/ursa-labs/crossbow doesn't exist. @kszucs Could you enable AppVeyor for https://github.com/ursa-labs/crossbow ? 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: us...@infra.apache.org
[GitHub] [arrow] carols10cents commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
carols10cents commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721941737 > I do not think that we should remove `impl PartialEq for BufferData`. I am only trying to understand why would we would like to compare two `Buffer` and not take the `ArrayData::offset` into account in the equality. Without the offset, comparing buffers is basically comparing physical memory, and IMO has limited use-cases. I was not proposing that we remove `impl PartialEq for BufferData`, I was only doing that to get the compiler to tell me about all the places we're using 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: us...@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
jorgecarleitao commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721939420 @carols10cents , I do not think that we should remove `impl PartialEq for BufferData`. I am only trying to understand why would we would like to compare two `Buffer` and not take the `ArrayData::offset` into account in the equality. Without the offset, comparing buffers is basically comparing physical memory, and IMO has limited use-cases. 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8591: ARROW-10484: [C++] Make Future<> more generic
github-actions[bot] commented on pull request #8591: URL: https://github.com/apache/arrow/pull/8591#issuecomment-721931117 https://issues.apache.org/jira/browse/ARROW-10484 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: us...@infra.apache.org
[GitHub] [arrow] bkietz opened a new pull request #8591: ARROW-10484: [C++] Make Future<> more generic
bkietz opened a new pull request #8591: URL: https://github.com/apache/arrow/pull/8591 `Future` and `Future` now support the `result()` member function; their `ValueType` is an empty struct. This will simplify generic code handling `Future` since special cases which avoid `result()` need not be written for statusy futures. Details: - `DeferNotOk` and `ExecuteAndMarkFinished` are no longer members of `Future<>` - Constructing finished Futures no longer locks the waiter mutex - Future now holds a `shared_ptr` directly. `FutureImpl` stores the Future's result in a type erased allocation. - `FutureStorage<>` and `FutureStorageBase` are obviated and have been removed - deleted `Executor::SubmitAsFuture()` since it isn't used and can be trivially replaced: `ex->SubmitAsFuture(f)` -> `DeferNotOk(ex->Submit(f))` 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: us...@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
jduo commented on pull request #8586: URL: https://github.com/apache/arrow/pull/8586#issuecomment-721902697 > LGTM, just one minor thing - can we link to the JIRA in the test and/or explain that the test was added to check for NPEs due to gRPC context keys having default values? Sure, I added some comments about what bug the test is testing specifically and how it can be encountered. 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: us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on a change in pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
kylepbit commented on a change in pull request #8572:
URL: https://github.com/apache/arrow/pull/8572#discussion_r517550295
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServer.java
##
@@ -351,6 +359,14 @@ public Builder headerAuthenticator(CallHeaderAuthenticator
headerAuthenticator)
return this;
}
+/**
+ * Set the header handler.
+ */
+public Builder headerHandler(ServerHeaderHandler headerHandler) {
Review comment:
As mentioned in another comment, the intent is to handle all headers.
You can interpret them as properties, but I think the name is reflective of
intent.
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:
us...@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #8056: ARROW-9861: [Java] Support big-endian in DecimalVector
BryanCutler commented on pull request #8056: URL: https://github.com/apache/arrow/pull/8056#issuecomment-721897680 merged to master, thanks @kiszk ! 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: us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on a change in pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
kylepbit commented on a change in pull request #8572:
URL: https://github.com/apache/arrow/pull/8572#discussion_r517544488
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import java.util.function.Consumer;
+
+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer {
Review comment:
There already is: see
https://github.com/apache/arrow/pull/8572/files#diff-b19522af7605b7cfe033ecc1300f2f6b0347d7eaba2969f0193086eb9751a546R102
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:
us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on a change in pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
kylepbit commented on a change in pull request #8572:
URL: https://github.com/apache/arrow/pull/8572#discussion_r517543577
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderHandler.java
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import java.util.function.Consumer;
+
+/**
+ * Interface for server side property handling.
+ */
+public interface ServerHeaderHandler extends Consumer {
Review comment:
This is not specific to properties - it's generally for handling all
headers so I think the name fits.
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:
us...@infra.apache.org
[GitHub] [arrow] kylepbit commented on a change in pull request #8572: ARROW-10467: [Java] Add the ability to pass arbitrary client headers.
kylepbit commented on a change in pull request #8572:
URL: https://github.com/apache/arrow/pull/8572#discussion_r517544006
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerHeaderMiddleware.java
##
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+/**
+ * Middleware that's used to extract and pass properties to the server during
requests.
+ */
+public class ServerHeaderMiddleware implements FlightServerMiddleware {
Review comment:
I think the name is appropriate given that this is intended to handle
all headers for calls, which could be considered properties depending on the
FlightServer implementation, but not necessarily.
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:
us...@infra.apache.org
[GitHub] [arrow] BryanCutler closed pull request #8056: ARROW-9861: [Java] Support big-endian in DecimalVector
BryanCutler closed pull request #8056: URL: https://github.com/apache/arrow/pull/8056 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: us...@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #8056: ARROW-9861: [Java] Support big-endian in DecimalVector
BryanCutler commented on pull request #8056: URL: https://github.com/apache/arrow/pull/8056#issuecomment-721893960 The test error with Java JNI looks unrelated and seems to be an env issue with ORC, I'll go ahead with merging this. 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: us...@infra.apache.org
[GitHub] [arrow] paddyhoran commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
paddyhoran commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721893832 Here is the PR: #4331 See the comment about half way down (I'm can't seem to link to it 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: us...@infra.apache.org
[GitHub] [arrow] paddyhoran commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
paddyhoran commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721891808 I can't find the reference now but this was discussed some time ago on a PR. The thinking for taking `capacity` into account when comparing `Buffer`'s was that array equality, etc. would be done at a higher level as @jorgecarleitao described. I think that @sunchao, in particular, favored taking `capacity` into account when comparing `Buffer`'s. i.e. this is a more low-level equality check. 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8589: ARROW-10493: [C++][Parquet] Fix offset lost in MaybeReplaceValidity
emkornfield commented on a change in pull request #8589:
URL: https://github.com/apache/arrow/pull/8589#discussion_r517514089
##
File path: cpp/src/parquet/arrow/arrow_reader_writer_test.cc
##
@@ -3199,6 +3199,27 @@ TEST(TestArrowReaderAdHoc, HandleDictPageOffsetZero) {
TryReadDataFile(test::get_data_file("dict-page-offset-zero.parquet"));
}
+TEST(TestArrowReaderAdHoc, WriteBatchedNestedNullableStringColumn) {
+ // ARROW-10493
+ auto string_array =
+ ::arrow::ArrayFromJSON(::arrow::utf8(), "[\"a\", \"b\", \"c\", \"d\"]");
Review comment:
array from json supports structs as {"column_name": value} entries, we
could clean this up a little but creating the type first and using
ArrayFromJson by itself without use StructArray::Make
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:
us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8589: ARROW-10493: [C++][Parquet] Fix offset lost in MaybeReplaceValidity
emkornfield commented on a change in pull request #8589: URL: https://github.com/apache/arrow/pull/8589#discussion_r517512787 ## File path: cpp/src/parquet/column_writer.cc ## @@ -1230,8 +1230,9 @@ class TypedColumnWriterImpl : public ColumnWriterImpl, public TypedColumnWriter< buffers[0] = bits_buffer_; // Should be a leaf array. DCHECK_EQ(array->num_fields(), 0); -return arrow::MakeArray(std::make_shared( -array->type(), array->length(), std::move(buffers), new_null_count)); +return arrow::MakeArray(std::make_shared(array->type(), array->length(), +std::move(buffers), Review comment: Nice catch. I think there is an edge case here. I think offset applies to the null buffer also, so this would cause an illegal access if any values are actually null. unfortunately, I think we need to slice all the buffers according to offset. 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: us...@infra.apache.org
[GitHub] [arrow] terencehonles commented on pull request #8386: ARROW-10224: [Python] Build, test, and support Python 3.9
terencehonles commented on pull request #8386: URL: https://github.com/apache/arrow/pull/8386#issuecomment-721863894 > Revision: [3d8adc2](https://github.com/apache/arrow/commit/3d8adc29623abae7e2cb29e5b2a77cc3161c12e8) > > Submitted crossbow builds: [ursa-labs/crossbow @ actions-687](https://github.com/ursa-labs/crossbow/branches/all?query=actions-687) > > Task Status > wheel-win-cp35m [](https://ci.appveyor.com/project/ursa-labs/crossbow/history) > wheel-win-cp36m [](https://ci.appveyor.com/project/ursa-labs/crossbow/history) > wheel-win-cp37m [](https://ci.appveyor.com/project/ursa-labs/crossbow/history) > wheel-win-cp38 [](https://ci.appveyor.com/project/ursa-labs/crossbow/history) > wheel-win-cp39 [](https://ci.appveyor.com/project/ursa-labs/crossbow/history) @kou any chance you know what happened to appveyor? 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: us...@infra.apache.org
[GitHub] [arrow] terencehonles commented on pull request #8533: ARROW-10080: [R] Call gc() and try again in MemoryPool
terencehonles commented on pull request #8533: URL: https://github.com/apache/arrow/pull/8533#issuecomment-721857972 > @terencehonles Sorry for the breakage! I added this check when debugging injection of the customized memory pool. I didn't see that `PoolBuffer::MakeUnique()` coerces null `MemoryPool*` to `default_memory_pool()`, so I assumed leaving it in would be harmless. It can be removed, though it's still slightly suspicious to me that null would be passed for a memory pool given that the test logs seem to indicate a different memory manager (`allocation manager type not specified, using netty as the default type`) was selected. No prob, I'll try to see if I can figure out what the Java code is doing, but thanks for the input! 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: us...@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8533: ARROW-10080: [R] Call gc() and try again in MemoryPool
bkietz commented on pull request #8533: URL: https://github.com/apache/arrow/pull/8533#issuecomment-721841024 @terencehonles Sorry for the breakage! I added this check when debugging injection of the customized memory pool. I didn't see that `PoolBuffer::MakeUnique()` coerces null `MemoryPool*` to `default_memory_pool()`, so I assumed leaving it in would be harmless. It can be removed, though it's still slightly suspicious to me that null would be passed for a memory pool given that the test logs seem to indicate a different memory manager (`allocation manager type not specified, using netty as the default type`) was selected. 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: us...@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8579: ARROW-10481: [R] Bindings to add, remove, replace Table columns
nealrichardson commented on a change in pull request #8579:
URL: https://github.com/apache/arrow/pull/8579#discussion_r517464426
##
File path: r/tests/testthat/test-Table.R
##
@@ -179,10 +183,34 @@ test_that("[[<- assignment", {
# can use $
tab$new <- NULL
expect_null(as.vector(tab$new))
- expect_identical(dim(tab), c(10L, 5L))
+ expect_identical(dim(tab), c(10L, 4L))
tab$int <- 1:10
expect_vector(tab$int, 1:10)
+
+ # recycling
+ tab[["atom"]] <- 1L
+ expect_vector(tab[["atom"]], rep(1L, 10))
+
+ expect_error(
+tab[["atom"]] <- 1:6,
+paste0(
+ "Invalid: Added column's length must match table's length. ",
+ "Expected length 10 but got length 6"
+)
+ )
+
+ # assign Arrow array and chunked_array
+ array <- Array$create(c(10:1))
+ tab$array <- array
+ expect_vector(tab$array, 10:1)
+
+ tab$chunked <- chunked_array(1:10)
+ expect_vector(tab$chunked, 1:10)
+
+ # nonsense indexes
+ expect_error(tab[[NA]] <- letters[10:1], "missing value where TRUE/FALSE
needed")
Review comment:
I'm not sure that's the right error to raise there so maybe there should
be better validation here.
Would also be important to test `NA_integer_` and `NA_real_` because those
will not get rejected by the R->cpp type mapping.
##
File path: r/tests/testthat/test-Table.R
##
@@ -179,10 +183,34 @@ test_that("[[<- assignment", {
# can use $
tab$new <- NULL
expect_null(as.vector(tab$new))
- expect_identical(dim(tab), c(10L, 5L))
+ expect_identical(dim(tab), c(10L, 4L))
tab$int <- 1:10
expect_vector(tab$int, 1:10)
+
+ # recycling
+ tab[["atom"]] <- 1L
+ expect_vector(tab[["atom"]], rep(1L, 10))
+
+ expect_error(
+tab[["atom"]] <- 1:6,
+paste0(
+ "Invalid: Added column's length must match table's length. ",
+ "Expected length 10 but got length 6"
+)
+ )
+
+ # assign Arrow array and chunked_array
+ array <- Array$create(c(10:1))
+ tab$array <- array
+ expect_vector(tab$array, 10:1)
+
+ tab$chunked <- chunked_array(1:10)
+ expect_vector(tab$chunked, 1:10)
+
+ # nonsense indexes
+ expect_error(tab[[NA]] <- letters[10:1], "missing value where TRUE/FALSE
needed")
+ expect_error(tab[[NULL]] <- letters[10:1], "argument is of length zero")
Review comment:
```suggestion
expect_error(tab[[NULL]] <- letters[10:1])
```
Since that is an error message coming from base R (right?), we shouldn't
assert its exact message because it may be translated to local language (i.e.
it only will pass if you're in an English locale).
##
File path: r/R/table.R
##
@@ -261,28 +261,38 @@ names.Table <- function(x) x$ColumnNames()
`[[<-.Table` <- function(x, i, value) {
if (is.null(value)) {
if (is.character(i)) {
- i <- match(i, names(x)) - 1L
+ i <- match(i, names(x))
}
-x <- x$RemoveColumn(i)
+x <- x$RemoveColumn(i - 1L)
} else {
if (!is.character(i)) {
# get or create a/the column name
- if (i <= ncol(x)) {
-i <- names(x)[[i]]
+ if (i <= x$num_columns) {
+i <- names(x)[i]
} else {
i <- as.character(i)
}
}
+# auto-magic recycling
Review comment:
1) We should probably only do recycling if `value` is not an ArrowObject
(perhaps with a TODO to support Arrow Scalar recycling)
2) https://vctrs.r-lib.org/reference/vec_recycle.html may be a good solution
instead of this bespoke code (we already import vctrs so it's available), or
else use `base::rep_len` (cf.
https://github.com/apache/arrow/blob/master/r/R/array.R#L291)
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:
us...@infra.apache.org
[GitHub] [arrow] terencehonles commented on pull request #8533: ARROW-10080: [R] Call gc() and try again in MemoryPool
terencehonles commented on pull request #8533: URL: https://github.com/apache/arrow/pull/8533#issuecomment-721835140 Here's some more context which could be helpful, the first coming from the run logs and the latter being a file that would be generated after the crash. Test logs: ``` [INFO] Running org.apache.arrow.adapter.orc.OrcReaderTest 16:21:58.149 [main] INFO org.apache.arrow.memory.BaseAllocator - Debug mode enabled. 16:21:58.172 [main] INFO org.apache.arrow.memory.DefaultAllocationManagerOption - allocation manager type not specified, using netty as the default type 16:21:58.175 [main] INFO org.apache.arrow.memory.CheckAllocator - Using DefaultAllocationManager at memory-netty/3.0.0-SNAPSHOT/arrow-memory-netty-3.0.0-SNAPSHOT.jar!/org/apache/arrow/memory/DefaultAllocationManagerFactory.class ``` Test crash logs: ``` # Created at 2020-11-04T16:22:01.347 WARNING: Logging before InitGoogleLogging() is written to STDERR # Created at 2020-11-04T16:22:01.347 F1104 16:22:01.345963 7719 memory.cc:57] Check failed: (pool) != (nullptr) # Created at 2020-11-04T16:22:01.347 *** Check failure stack trace: *** # Created at 2020-11-04T16:22:04.748 Aborted (core dumped) ``` 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: us...@infra.apache.org
[GitHub] [arrow] jorgecarleitao edited a comment on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
jorgecarleitao edited a comment on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721818365 Could you describe where do we need to compare buffers? I am asking because I have worked on this problem before, and in my last iteration of this, I abandoned the idea of comparing buffers: a `Buffer` is purely a physical representation of data without any logical interpretation: two buffers from two different arrays can be equal and the arrays still be different (e.g. due to the nullability or offset), and the opposite is also true: two arrays can be equal but have different buffers (e.g. if the child data is different, or if the datatype is different). My current hypothesis is that equality should be done via `ArrayData`, which contains all the relevant data _and_ logical representation of that data -- `DataType`, to make it possible to logically compare two arrays. I fielded #8541 with that idea, which IMO will also help the parquet work. I think that @nevi-me also had some ideas in mind. 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: us...@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
jduo commented on pull request #8586: URL: https://github.com/apache/arrow/pull/8586#issuecomment-721821821 FYI @lidavidm , this got introduced during the auth 2 redesign. 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: us...@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8586: ARROW-10491: [FlightRPC][Java] Fix NPE when using makeContext
jduo commented on pull request #8586: URL: https://github.com/apache/arrow/pull/8586#issuecomment-721821191 > Changes look good, is it possible to add tests for this change? Test added. 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: us...@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
jorgecarleitao commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721818365 Could you describe where do we need to compare buffers? I am asking because I have worked on this problem before, and in my last iteration of this, I abandoned the idea of comparing buffers: a `Buffer` is purely a physical representation of data without any logical interpretation: two buffers from two different arrays can be equal and the arrays still be different (e.g. due to the nullability), and the opposite is also true: two arrays can be equal but have different buffers (e.g. if the child data is different, or if the datatype is different). IMO the equality should be done via `ArrayData`, which contains all the relevant data _and_ logical representation of that data -- `DataType`, to make it possible to logically compare two arrays. I fielded #8541 with that idea, which IMO will also help the parquet work. I think that @nevi-me also had some ideas in mind. 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: us...@infra.apache.org
[GitHub] [arrow] terencehonles edited a comment on pull request #8533: ARROW-10080: [R] Call gc() and try again in MemoryPool
terencehonles edited a comment on pull request #8533:
URL: https://github.com/apache/arrow/pull/8533#issuecomment-721815501
> CI failures are unrelated. Merging
@bkietz I've bisected the commit history, and it does look like [this
change](https://github.com/apache/arrow/pull/8533/files#diff-2b5392b203328a6b9e7ff692e51eb39691cc1d05329882bef0200f0d9bd51f84R57)
```diff
diff --git a/cpp/src/arrow/io/memory.cc b/cpp/src/arrow/io/memory.cc
index 1cde5e64e..88f006fe0 100644
--- a/cpp/src/arrow/io/memory.cc
+++ b/cpp/src/arrow/io/memory.cc
@@ -54,7 +54,7 @@ BufferOutputStream::BufferOutputStream(const
std::shared_ptr& b
Result> BufferOutputStream::Create(
int64_t initial_capacity, MemoryPool* pool) {
// ctor is private, so cannot use make_shared
+ DCHECK_NE(pool, nullptr);
auto ptr = std::shared_ptr(new BufferOutputStream);
RETURN_NOT_OK(ptr->Reset(initial_capacity, pool));
return ptr;
```
is "responsible" for breaking the JNI status check (you can also see it was
passing on the first commit in this PR
https://github.com/apache/arrow/runs/1310578738 and then failing at the end
https://github.com/apache/arrow/runs/1329149428).
I am not familiar with the code, and I was hoping for you to possibly help
me understand why the check was added and why it might be segfaulting the test
java/adapter/orc/src/test/java/org/apache/arrow/adapter/orc/OrcReaderTest.java.
I have experimented with removing the check and the test passes as expected,
but if the check was added to catch something I'd rather not cover it up. I
noticed there were two follow ups ARROW-10420 and ARROW-10421 and I was
wondering if this might be related?
Edit: since the runs above were not merged and their branch was deleted it
looks like GitHub dropped the logs. This is the specific test error I am
referring to (grabbed from a run on the `master` branch)
https://github.com/apache/arrow/runs/1351819845#step:8:7374
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:
us...@infra.apache.org
[GitHub] [arrow] terencehonles commented on pull request #8533: ARROW-10080: [R] Call gc() and try again in MemoryPool
terencehonles commented on pull request #8533:
URL: https://github.com/apache/arrow/pull/8533#issuecomment-721815501
> CI failures are unrelated. Merging
@bkietz I've bisected the commit history, and it does look like [this
change](https://github.com/apache/arrow/pull/8533/files#diff-2b5392b203328a6b9e7ff692e51eb39691cc1d05329882bef0200f0d9bd51f84R57)
```diff
diff --git a/cpp/src/arrow/io/memory.cc b/cpp/src/arrow/io/memory.cc
index 1cde5e64e..88f006fe0 100644
--- a/cpp/src/arrow/io/memory.cc
+++ b/cpp/src/arrow/io/memory.cc
@@ -54,7 +54,7 @@ BufferOutputStream::BufferOutputStream(const
std::shared_ptr& b
Result> BufferOutputStream::Create(
int64_t initial_capacity, MemoryPool* pool) {
// ctor is private, so cannot use make_shared
+ DCHECK_NE(pool, nullptr);
auto ptr = std::shared_ptr(new BufferOutputStream);
RETURN_NOT_OK(ptr->Reset(initial_capacity, pool));
return ptr;
```
is "responsible" for breaking the JNI status check (you can also see it was
passing on the first commit in this PR
https://github.com/apache/arrow/runs/1310578738 and then failing at the end
https://github.com/apache/arrow/runs/1329149428).
I am not familiar with the code, and I was hoping for you to possibly help
me understand why the check was added and why it might be segfaulting the test
java/adapter/orc/src/test/java/org/apache/arrow/adapter/orc/OrcReaderTest.java.
I have experimented with removing the check and the test passes as expected,
but if the check was added to catch something I'd rather not cover it up. I
noticed there were two follow ups ARROW-10420 and ARROW-10421 and I was
wondering if this might be related?
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:
us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
github-actions[bot] commented on pull request #8590: URL: https://github.com/apache/arrow/pull/8590#issuecomment-721810399 https://issues.apache.org/jira/browse/ARROW-10042 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8386: ARROW-10224: [Python] Build, test, and support Python 3.9
github-actions[bot] commented on pull request #8386: URL: https://github.com/apache/arrow/pull/8386#issuecomment-721809007 Revision: 3d8adc29623abae7e2cb29e5b2a77cc3161c12e8 Submitted crossbow builds: [ursa-labs/crossbow @ actions-687](https://github.com/ursa-labs/crossbow/branches/all?query=actions-687) |Task|Status| ||--| |wheel-win-cp35m|[](https://ci.appveyor.com/project/ursa-labs/crossbow/history)| |wheel-win-cp36m|[](https://ci.appveyor.com/project/ursa-labs/crossbow/history)| |wheel-win-cp37m|[](https://ci.appveyor.com/project/ursa-labs/crossbow/history)| |wheel-win-cp38|[](https://ci.appveyor.com/project/ursa-labs/crossbow/history)| |wheel-win-cp39|[](https://ci.appveyor.com/project/ursa-labs/crossbow/history)| 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: us...@infra.apache.org
[GitHub] [arrow] terencehonles commented on pull request #8386: ARROW-10224: [Python] Build, test, and support Python 3.9
terencehonles commented on pull request #8386: URL: https://github.com/apache/arrow/pull/8386#issuecomment-721808090 @github-actions crossbow submit wheel-win-* 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: us...@infra.apache.org
[GitHub] [arrow] carols10cents opened a new pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality
carols10cents opened a new pull request #8590: URL: https://github.com/apache/arrow/pull/8590 And disregard differences in capacity. This gets 2 ignored Parquet -> Arrow roundtrip tests to pass because they were only failing due to differences in `BufferData` capacities. I noticed that `MutableBuffer`'s `PartialEq` also checks `capacity`, and I think that's still correct: two `MutableBuffer`s who differ only in `capacity` could have different behavior if you try to add the same data to each of them and one doesn't have sufficient capacity. 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: us...@infra.apache.org
[GitHub] [arrow] jonkeane commented on pull request #8579: ARROW-10481: [R] Bindings to add, remove, replace Table columns
jonkeane commented on pull request #8579: URL: https://github.com/apache/arrow/pull/8579#issuecomment-721785580 I added the extra tests and some code to get that working. I also started down the path of `[<-.Table` though there are more decisions that are under that rock (what to do about row manipulation, etc.), so left it be for now. It currently errors in R (though we probably want to write a more helpful message as part of this PR even if we kick that working to another branch/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: us...@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #8579: ARROW-10481: [R] Bindings to add, remove, replace Table columns
jonkeane commented on a change in pull request #8579:
URL: https://github.com/apache/arrow/pull/8579#discussion_r517366041
##
File path: r/R/table.R
##
@@ -254,6 +257,49 @@ names.Table <- function(x) x$ColumnNames()
#' @export
`[[.Table` <- `[[.RecordBatch`
+#' @export
+`[[<-.Table` <- function(x, i, value) {
+ if (is.null(value)) {
+if (is.character(i)) {
+ i <- match(i, names(x)) - 1L
+}
+x <- x$RemoveColumn(i)
+ } else {
+if (!is.character(i)) {
+ # get or create a/the column name
+ if (i <= ncol(x)) {
Review comment:
I went looking for a function call to `num_columns()` and should have
looked for an active binding, oops!
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:
us...@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8581: ARROW-10441: [Java] Prevent closure of shared channels for FlightClient
lidavidm commented on a change in pull request #8581: URL: https://github.com/apache/arrow/pull/8581#discussion_r517340629 ## File path: java/flight/flight-grpc/src/main/java/org/apache/arrow/flight/FlightGrpcUtils.java ## @@ -49,10 +143,10 @@ public static BindableService createFlightService(BufferAllocator allocator, Fli /** * Creates a Flight client. * @param incomingAllocator Memory allocator - * @param channel provides a connection to a gRPC server + * @param channel provides a connection to a gRPC server. Will not be closed on closure of the returned FlightClient. Review comment: Yes - how does that not solve the problem? The caller can choose whether the FlightClient takes ownership of the channel or not by calling different methods. But if the current method is modified, then any existing code will suddenly silently leak channels. 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8589: ARROW-10493: [C++][Parquet] Fix offset lost in MaybeReplaceValidity
github-actions[bot] commented on pull request #8589: URL: https://github.com/apache/arrow/pull/8589#issuecomment-721637384 https://issues.apache.org/jira/browse/ARROW-10493 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: us...@infra.apache.org
[GitHub] [arrow] chrisavl opened a new pull request #8589: ARROW-10493: [C++][Parquet] Fix offset lost in MaybeReplaceValidity
chrisavl opened a new pull request #8589: URL: https://github.com/apache/arrow/pull/8589 This PR fixes the offset being lost when the validity bitmap is updated. 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: us...@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8588: ARROW-4637: [Python] Must lock on import, to avoid deadlock due to circular imports
github-actions[bot] commented on pull request #8588: URL: https://github.com/apache/arrow/pull/8588#issuecomment-721627353 https://issues.apache.org/jira/browse/ARROW-4637 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: us...@infra.apache.org
[GitHub] [arrow] malthe edited a comment on pull request #3893: ARROW-4637: [Python] Conditionally import pandas symbols if they are used. Do not require pandas as a test dependency
malthe edited a comment on pull request #3893: URL: https://github.com/apache/arrow/pull/3893#issuecomment-721603457 The downside of this fix is that it causes import deadlocking if using Pandas in multithreaded mode. I have opened a pull request to address this: #8588. 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: us...@infra.apache.org
[GitHub] [arrow] malthe opened a new pull request #8588: ARROW-4637: [Python] Must lock on import, to avoid deadlock due to circular imports
malthe opened a new pull request #8588: URL: https://github.com/apache/arrow/pull/8588 This fixes an issue where multithreaded use of the library would cause import deadlock issues due to the lazy Pandas importing mechanism. 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: us...@infra.apache.org
[GitHub] [arrow] vertexclique commented on a change in pull request #8561: ARROW-10449 [Rust] Make Dictionary::keys be an array
vertexclique commented on a change in pull request #8561:
URL: https://github.com/apache/arrow/pull/8561#discussion_r517199502
##
File path: rust/arrow/src/array/array.rs
##
@@ -2319,13 +2221,24 @@ impl From for
DictionaryArray {
"DictionaryArray should contain a single child array (values)."
);
-let raw_values = data.buffers()[0].raw_data();
-let dtype: = data.data_type();
-let values = make_array(data.child_data()[0].clone());
-if let DataType::Dictionary(_, _) = dtype {
+if let DataType::Dictionary(key_data_type, _) = data.data_type() {
+if key_data_type.as_ref() != ::DATA_TYPE {
+panic!("DictionaryArray's data type must match.")
Review comment:
I don't think that unreachable is only for the inconsistent state. But
we can leave it as panic here. I was also unsure about the user-facing API's
panicking behavior. Especially in array methods with forced asserts. We should
prefer Result than direct asserts, but you know... That's also yet another
topic.
##
File path: rust/arrow/src/array/array.rs
##
@@ -2398,30 +2311,23 @@ impl Array for
DictionaryArray {
/// Returns the total number of bytes of memory occupied by the buffers
owned by this [DictionaryArray].
fn get_buffer_memory_size() -> usize {
-self.data.get_buffer_memory_size() +
self.values().get_buffer_memory_size()
+// Since both `keys` and `values` derive (are references from) `data`,
we only account for `data`.
+self.data.get_array_memory_size()
Review comment:
Voila! Yes.
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:
us...@infra.apache.org
[GitHub] [arrow] vertexclique commented on a change in pull request #8561: ARROW-10449 [Rust] Make Dictionary::keys be an array
vertexclique commented on a change in pull request #8561:
URL: https://github.com/apache/arrow/pull/8561#discussion_r517197699
##
File path: rust/arrow/src/array/array.rs
##
@@ -2145,112 +2149,10 @@ pub struct DictionaryArray {
is_ordered: bool,
}
-#[derive(Debug)]
-enum Draining {
-Ready,
-Iterating,
-Finished,
-}
-
-#[derive(Debug)]
-pub struct NullableIter<'a, T> {
-data: &'a ArrayDataRef, // TODO: Use a pointer to the null bitmap.
-ptr: *const T,
-i: usize,
-len: usize,
-draining: Draining,
-}
-
-impl<'a, T> std::iter::Iterator for NullableIter<'a, T>
-where
-T: Clone,
-{
-type Item = Option;
-
-fn next( self) -> Option {
-let i = self.i;
-if i >= self.len {
-None
-} else if self.data.is_null(i) {
-self.i += 1;
-Some(None)
-} else {
-self.i += 1;
-unsafe { Some(Some((&*self.ptr.add(i)).clone())) }
-}
-}
-
-fn size_hint() -> (usize, Option) {
-(self.len, Some(self.len))
-}
-
-fn nth( self, n: usize) -> Option {
-let i = self.i;
-if i + n >= self.len {
-self.i = self.len;
-None
-} else if self.data.is_null(i + n) {
-self.i += n + 1;
-Some(None)
-} else {
-self.i += n + 1;
-unsafe { Some(Some((&*self.ptr.add(i + n)).clone())) }
-}
-}
-}
-
-impl<'a, T> std::iter::DoubleEndedIterator for NullableIter<'a, T>
-where
-T: Clone,
-{
-fn next_back( self) -> Option {
-match self.draining {
-Draining::Ready => {
-self.draining = Draining::Iterating;
-self.i = self.len - 1;
-self.next_back()
-}
-Draining::Iterating => {
-let i = self.i;
-if i >= self.len {
-None
-} else if self.data.is_null(i) {
-self.i = self.i.checked_sub(1).unwrap_or_else(|| {
-self.draining = Draining::Finished;
-0_usize
-});
-Some(None)
-} else {
-match i.checked_sub(1) {
-Some(idx) => {
-self.i = idx;
-unsafe { Some(Some((&*self.ptr.add(i)).clone())) }
-}
-_ => {
-self.draining = Draining::Finished;
-unsafe { Some(Some((&*self.ptr).clone())) }
-}
-}
-}
-}
-Draining::Finished => {
-self.draining = Draining::Ready;
-None
-}
-}
-}
-}
-
impl<'a, K: ArrowPrimitiveType> DictionaryArray {
/// Return an iterator to the keys of this dictionary.
-pub fn keys() -> NullableIter<'_, K::Native> {
-NullableIter::<'_, K::Native> {
-data: ,
-ptr: unsafe { self.raw_values.get().add(self.data.offset()) },
-i: 0,
-len: self.data.len(),
-draining: Draining::Ready,
-}
+pub fn keys() -> {
Review comment:
me too. that's way 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:
us...@infra.apache.org
[GitHub] [arrow] malthe commented on pull request #3893: ARROW-4637: [Python] Conditionally import pandas symbols if they are used. Do not require pandas as a test dependency
malthe commented on pull request #3893: URL: https://github.com/apache/arrow/pull/3893#issuecomment-721603457 The downside of this fix is that it causes import deadlocking if using Pandas in multithreaded mode. 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: us...@infra.apache.org
[GitHub] [arrow] praveenbingo closed pull request #8555: ARROW-9897: [C++][Gandiva] Revert - to_date function
praveenbingo closed pull request #8555: URL: https://github.com/apache/arrow/pull/8555 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: us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8583: Arrow 10427: [Flight][Java] Add optional session header
jduo commented on a change in pull request #8583:
URL: https://github.com/apache/arrow/pull/8583#discussion_r517168909
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientSessionMiddleware.java
##
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight.client;
+
+import org.apache.arrow.flight.CallHeaders;
+import org.apache.arrow.flight.CallInfo;
+import org.apache.arrow.flight.CallStatus;
+import org.apache.arrow.flight.FlightClientMiddleware;
+import org.apache.arrow.flight.FlightConstants;
+
+/**
+ * Client middleware to re-use the existing session provided by the server.
+ */
+public class ClientSessionMiddleware implements FlightClientMiddleware {
Review comment:
I think the ClientSessionMiddleware should be responsible for sending
the header back rather than sending the header as part of a CallOption.
The reason is that you are leaving it up to the application code to check
the expiration of the session. (Note that a positive max-age can still expire
after some time).
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:
us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8583: Arrow 10427: [Flight][Java] Add optional session header
jduo commented on a change in pull request #8583:
URL: https://github.com/apache/arrow/pull/8583#discussion_r517166976
##
File path:
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestServerSessionHandling.java
##
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.arrow.flight.grpc.RequestContextAdapter;
+import org.junit.Test;
+
+import com.google.common.collect.ImmutableList;
+
+/**
+ * Tests for ServerSessionHandler and ServerSessionMiddleware.
+ */
+public class TestServerSessionHandling {
+ private static final String SESSION_HEADER_KEY = "Session";
+ private static final String SET_SESSION_HEADER_KEY = "Set-Session";
+ private static final String TEST_SESSION =
"name=test-session;id=session-id;max-age=100";
+ private static final List VALID_PROPERTY_KEYS = ImmutableList.of(
+ "name",
+ "id",
+ "max-age"
+ );
+
+ private class SimpleServerSessionHandler implements ServerSessionHandler {
+private String session;
+
+SimpleServerSessionHandler() {
+ this.session = null;
+}
+
+@Override
+public String beginSession(CallHeaders headers) {
+ session = TEST_SESSION;
+ return session;
+}
+
+@Override
+public String getSession(CallHeaders headers) {
+ // No session has been set yet
+ if (session == null) {
+return null;
+ }
+
+ // Expected session header but none is found
+ if (!headers.containsKey(SESSION_HEADER_KEY)) {
Review comment:
This code to parse the header should not be done by the implementation
of the interface since we are generating and transmitting this header in the
Flight layer (so this logic isn't FlightProducer-implementation-specific).
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:
us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8583: Arrow 10427: [Flight][Java] Add optional session header
jduo commented on a change in pull request #8583:
URL: https://github.com/apache/arrow/pull/8583#discussion_r517165633
##
File path:
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestServerSessionHandling.java
##
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.arrow.flight.grpc.RequestContextAdapter;
+import org.junit.Test;
+
+import com.google.common.collect.ImmutableList;
+
+/**
+ * Tests for ServerSessionHandler and ServerSessionMiddleware.
+ */
+public class TestServerSessionHandling {
+ private static final String SESSION_HEADER_KEY = "Session";
+ private static final String SET_SESSION_HEADER_KEY = "Set-Session";
+ private static final String TEST_SESSION =
"name=test-session;id=session-id;max-age=100";
+ private static final List VALID_PROPERTY_KEYS = ImmutableList.of(
+ "name",
+ "id",
+ "max-age"
+ );
+
+ private class SimpleServerSessionHandler implements ServerSessionHandler {
+private String session;
+
+SimpleServerSessionHandler() {
+ this.session = null;
+}
+
+@Override
+public String beginSession(CallHeaders headers) {
+ session = TEST_SESSION;
+ return session;
+}
+
+@Override
+public String getSession(CallHeaders headers) {
+ // No session has been set yet
+ if (session == null) {
+return null;
+ }
+
+ // Expected session header but none is found
+ if (!headers.containsKey(SESSION_HEADER_KEY)) {
+throw CallStatus.NOT_FOUND.toRuntimeException();
+ }
+
+ // Validate that session header does not contain invalid properties
+ final Map propertiesMap = new HashMap<>();
+ final String[] inSessionProperties =
headers.get(SESSION_HEADER_KEY).split(";");
+ for (String pair : inSessionProperties) {
+final String[] keyVal = pair.split("=");
Review comment:
Is '=' a legal character for a value?
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:
us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8583: Arrow 10427: [Flight][Java] Add optional session header
jduo commented on a change in pull request #8583:
URL: https://github.com/apache/arrow/pull/8583#discussion_r517164883
##
File path:
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestServerSessionHandling.java
##
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.arrow.flight.grpc.RequestContextAdapter;
+import org.junit.Test;
+
+import com.google.common.collect.ImmutableList;
+
+/**
+ * Tests for ServerSessionHandler and ServerSessionMiddleware.
+ */
+public class TestServerSessionHandling {
+ private static final String SESSION_HEADER_KEY = "Session";
+ private static final String SET_SESSION_HEADER_KEY = "Set-Session";
+ private static final String TEST_SESSION =
"name=test-session;id=session-id;max-age=100";
+ private static final List VALID_PROPERTY_KEYS = ImmutableList.of(
+ "name",
+ "id",
+ "max-age"
+ );
+
+ private class SimpleServerSessionHandler implements ServerSessionHandler {
+private String session;
+
+SimpleServerSessionHandler() {
+ this.session = null;
+}
+
+@Override
+public String beginSession(CallHeaders headers) {
+ session = TEST_SESSION;
+ return session;
+}
+
+@Override
+public String getSession(CallHeaders headers) {
+ // No session has been set yet
+ if (session == null) {
+return null;
+ }
+
+ // Expected session header but none is found
+ if (!headers.containsKey(SESSION_HEADER_KEY)) {
+throw CallStatus.NOT_FOUND.toRuntimeException();
+ }
+
+ // Validate that session header does not contain invalid properties
+ final Map propertiesMap = new HashMap<>();
+ final String[] inSessionProperties =
headers.get(SESSION_HEADER_KEY).split(";");
+ for (String pair : inSessionProperties) {
+final String[] keyVal = pair.split("=");
+if (!VALID_PROPERTY_KEYS.contains(keyVal[0])) {
+ throw CallStatus.INVALID_ARGUMENT.toRuntimeException();
+}
+propertiesMap.put(keyVal[0], keyVal[1]);
+ }
+
+ // Validate that session has not expired
+ final int maxAge = Integer.parseInt(propertiesMap.get("max-age"));
+ if (maxAge == 0) {
Review comment:
For cookies, max-age < 0 also means it is expired.
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:
us...@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8583: Arrow 10427: [Flight][Java] Add optional session header
jduo commented on a change in pull request #8583:
URL: https://github.com/apache/arrow/pull/8583#discussion_r517156055
##
File path:
java/flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientSessionMiddleware.java
##
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.flight.client;
+
+import org.apache.arrow.flight.CallHeaders;
+import org.apache.arrow.flight.CallInfo;
+import org.apache.arrow.flight.CallStatus;
+import org.apache.arrow.flight.FlightClientMiddleware;
+import org.apache.arrow.flight.FlightConstants;
+
+/**
+ * Client middleware to re-use the existing session provided by the server.
+ */
+public class ClientSessionMiddleware implements FlightClientMiddleware {
+ /**
+ * Factory to create instances of ClientSessionMiddleware for each call.
+ */
+ public static class Factory implements FlightClientMiddleware.Factory {
+private final ClientSessionWriter sessionWriter;
+
+public Factory() {
+ sessionWriter = new ClientSessionWriter();
+}
+
+@Override
+public FlightClientMiddleware onCallStarted(CallInfo info) {
+ return new ClientSessionMiddleware(sessionWriter);
+}
+ }
+
+ private final ClientSessionWriter sessionWriter;
+
+ public ClientSessionMiddleware(ClientSessionWriter sessionWriter) {
Review comment:
This shouldn't be public. The factory should be the only thing that can
create these, outside of testing.
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:
us...@infra.apache.org
