Skip to content

Commit 0924124

Browse files
lriggsFiV0kouraulcd
authored
Cherry pick fix for empty vectors and some additional build process fixes. (dremio#81)
* apacheGH-30866: [Java] fix SplitAndTransfer throws for (0,0) if vector empty (apache#41066) This is addresses https://issues.apache.org/jira/browse/ARROW-15382 and is reopening of apache#12250 (which I asked to be reopened). I tried to address all the comments from the previous discussion, added some more tests and fixed an issue in the old commit. * GitHub Issue: apache#30866 Authored-by: Finn Völkel <finn.volkel@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com> * apacheGH-43463: [C++][Gandiva] Always use gdv_function_stubs.h in context_helper.cc (apache#43464) ### Rationale for this change `gdv_function_stubs.h` has declarations of functions in `context_helper.cc`. If we don't include `gdv_function_stubs.h`, it causes attribution mismatch error with unity build. ### What changes are included in this PR? Always include `gdv_function_stubs.h` in `context_helper.cc`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#43463 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> * apacheGH-43119: [CI][Packaging] Update manylinux 2014 CentOS repos that have been deprecated (apache#43121) ### Rationale for this change Jobs are failing to find mirrorlist.centos.org ### What changes are included in this PR? Updating repos based on solution from: apache#43119 (comment) ### Are these changes tested? Via archery ### Are there any user-facing changes? No * GitHub Issue: apache#43119 Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com> * Update macos deployment target to 12 to match build machine. * apacheGH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest (apache#43465) ### Rationale for this change If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be: * `-isystem /opt/homebrew/include` (for Boost) * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) With this order, GoogleTest headers in `/opt/homebrew/include/` are used with bundled GoogleTest. It may cause link errors. ### What changes are included in this PR? This change introduces a new CMake target `arrow::GTest::gtest_headers` that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are: * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) * `-isystem /opt/homebrew/include` (for Boost) With this order, we can always use our bundled GoogleTest. `arrow::GTest::gtest_headers` is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#43400 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de> --------- Signed-off-by: David Li <li.davidm96@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de> Co-authored-by: Finn Völkel <FiV0@users.noreply.github.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
1 parent ed7efa2 commit 0924124

11 files changed

Lines changed: 181 additions & 35 deletions

File tree

‎ci/docker/centos-7-cpp.dockerfile‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,25 @@
1717

1818
FROM centos:centos7
1919

20+
# Update mirrors to use vault.centos.org as CentOS 7
21+
# is EOL since 2024-06-30
22+
RUN sed -i \
23+
-e 's/^mirrorlist/#mirrorlist/' \
24+
-e 's/^#baseurl/baseurl/' \
25+
-e 's/mirror\.centos\.org/vault.centos.org/' \
26+
/etc/yum.repos.d/*.repo
27+
2028
# devtoolset is required for C++17
2129
RUN \
2230
yum install -y \
2331
centos-release-scl \
2432
epel-release && \
33+
sed -i \
34+
-e 's/^mirrorlist/#mirrorlist/' \
35+
-e 's/^#baseurl/baseurl/' \
36+
-e 's/^# baseurl/baseurl/' \
37+
-e 's/mirror\.centos\.org/vault.centos.org/' \
38+
/etc/yum.repos.d/CentOS-SCLo-scl*.repo && \
2539
yum install -y \
2640
cmake3 \
2741
curl \

‎ci/docker/python-wheel-manylinux.dockerfile‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,18 @@ ARG manylinux
2525
ENV MANYLINUX_VERSION=${manylinux}
2626

2727
# Ensure dnf is installed, especially for the manylinux2014 base
28+
RUN if [ "${MANYLINUX_VERSION}" = "2014" ]; then \
29+
sed -i \
30+
-e 's/^mirrorlist/#mirrorlist/' \
31+
-e 's/^#baseurl/baseurl/' \
32+
-e 's/mirror\.centos\.org/vault.centos.org/' \
33+
/etc/yum.repos.d/*.repo; \
34+
if [ "${arch}" != "amd64" ]; then \
35+
sed -i \
36+
-e 's,vault\.centos\.org/centos,vault.centos.org/altarch,' \
37+
/etc/yum.repos.d/CentOS-SCLo-scl-rh.repo; \
38+
fi; \
39+
fi
2840
RUN yum install -y dnf
2941

3042
# Install basic dependencies

‎cpp/cmake_modules/BuildUtils.cmake‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -721,6 +721,11 @@ function(ADD_TEST_CASE REL_TEST_NAME)
721721
"${EXECUTABLE_OUTPUT_PATH};$ENV{CONDA_PREFIX}/lib")
722722
endif()
723723

724+
# Ensure using bundled GoogleTest when we use bundled GoogleTest.
725+
# ARROW_GTEST_GTEST_HEADERS is defined only when we use bundled
726+
# GoogleTest.
727+
target_link_libraries(${TEST_NAME} PRIVATE ${ARROW_GTEST_GTEST_HEADERS})
728+
724729
if(ARG_STATIC_LINK_LIBS)
725730
# Customize link libraries
726731
target_link_libraries(${TEST_NAME} PRIVATE ${ARG_STATIC_LINK_LIBS})

‎cpp/cmake_modules/ThirdpartyToolchain.cmake‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2298,6 +2298,10 @@ function(build_gtest)
22982298
install(DIRECTORY "${googletest_SOURCE_DIR}/googlemock/include/"
22992299
"${googletest_SOURCE_DIR}/googletest/include/"
23002300
DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
2301+
add_library(arrow::GTest::gtest_headers INTERFACE IMPORTED)
2302+
target_include_directories(arrow::GTest::gtest_headers
2303+
INTERFACE "${googletest_SOURCE_DIR}/googlemock/include/"
2304+
"${googletest_SOURCE_DIR}/googletest/include/")
23012305
install(TARGETS gmock gmock_main gtest gtest_main
23022306
EXPORT arrow_testing_targets
23032307
RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
@@ -2342,12 +2346,14 @@ if(ARROW_TESTING)
23422346

23432347
string(APPEND ARROW_TESTING_PC_LIBS " $<TARGET_FILE:GTest::gtest>")
23442348
endif()
2349+
set(ARROW_GTEST_GTEST_HEADERS)
23452350
set(ARROW_GTEST_GMOCK GTest::gmock)
23462351
set(ARROW_GTEST_GTEST GTest::gtest)
23472352
set(ARROW_GTEST_GTEST_MAIN GTest::gtest_main)
23482353
else()
23492354
string(APPEND ARROW_TESTING_PC_CFLAGS " -I\${includedir}/arrow-gtest")
23502355
string(APPEND ARROW_TESTING_PC_LIBS " -larrow_gtest")
2356+
set(ARROW_GTEST_GTEST_HEADERS arrow::GTest::gtest_headers)
23512357
set(ARROW_GTEST_GMOCK arrow::GTest::gmock)
23522358
set(ARROW_GTEST_GTEST arrow::GTest::gtest)
23532359
set(ARROW_GTEST_GTEST_MAIN arrow::GTest::gtest_main)

‎cpp/src/gandiva/context_helper.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
#include "gandiva/execution_context.h"
19+
#include "gandiva/gdv_function_stubs.h"
20+
1821
// This file is also used in the pre-compiled unit tests, which do include
1922
// llvm/engine/..
2023
#ifndef GANDIVA_UNIT_TEST
2124
#include "gandiva/exported_funcs.h"
22-
#include "gandiva/gdv_function_stubs.h"
2325

2426
#include "gandiva/engine.h"
2527

@@ -56,8 +58,6 @@ arrow::Status ExportedContextFunctions::AddMappings(Engine* engine) const {
5658
} // namespace gandiva
5759
#endif // !GANDIVA_UNIT_TEST
5860

59-
#include "gandiva/execution_context.h"
60-
6161
extern "C" {
6262

6363
void gdv_fn_context_set_error_msg(int64_t context_ptr, char const* err_msg) {

‎dev/tasks/java-jars/github.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ jobs:
9595
platform:
9696
- { runs_on: ["macos-12"], arch: "x86_64"}
9797
env:
98-
MACOSX_DEPLOYMENT_TARGET: "10.15"
98+
MACOSX_DEPLOYMENT_TARGET: "12.0"
9999
steps:
100100
{{ macros.github_checkout_arrow()|indent }}
101101
- name: Set up Python

‎java/vector/src/main/java/org/apache/arrow/vector/BaseLargeVariableWidthVector.java‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -763,16 +763,14 @@ public void transferTo(BaseLargeVariableWidthVector target) {
763763
*/
764764
public void splitAndTransferTo(int startIndex, int length,
765765
BaseLargeVariableWidthVector target) {
766-
Preconditions.checkArgument(startIndex >= 0 && startIndex < valueCount,
767-
"Invalid startIndex: %s", startIndex);
768-
Preconditions.checkArgument(startIndex + length <= valueCount,
769-
"Invalid length: %s", length);
766+
Preconditions.checkArgument(startIndex >= 0 && length >= 0 && startIndex + length <= valueCount,
767+
"Invalid parameters startIndex: %s, length: %s for valueCount: %s", startIndex, length, valueCount);
770768
compareTypes(target, "splitAndTransferTo");
771769
target.clear();
772-
splitAndTransferValidityBuffer(startIndex, length, target);
773-
splitAndTransferOffsetBuffer(startIndex, length, target);
774-
target.setLastSet(length - 1);
775770
if (length > 0) {
771+
splitAndTransferValidityBuffer(startIndex, length, target);
772+
splitAndTransferOffsetBuffer(startIndex, length, target);
773+
target.setLastSet(length - 1);
776774
target.setValueCount(length);
777775
}
778776
}

‎java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthVector.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -808,10 +808,10 @@ public void splitAndTransferTo(int startIndex, int length,
808808
"Invalid parameters startIndex: %s, length: %s for valueCount: %s", startIndex, length, valueCount);
809809
compareTypes(target, "splitAndTransferTo");
810810
target.clear();
811-
splitAndTransferValidityBuffer(startIndex, length, target);
812-
splitAndTransferOffsetBuffer(startIndex, length, target);
813-
target.setLastSet(length - 1);
814811
if (length > 0) {
812+
splitAndTransferValidityBuffer(startIndex, length, target);
813+
splitAndTransferOffsetBuffer(startIndex, length, target);
814+
target.setLastSet(length - 1);
815815
target.setValueCount(length);
816816
}
817817
}

‎java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -552,21 +552,23 @@ public void transfer() {
552552
public void splitAndTransfer(int startIndex, int length) {
553553
Preconditions.checkArgument(startIndex >= 0 && length >= 0 && startIndex + length <= valueCount,
554554
"Invalid parameters startIndex: %s, length: %s for valueCount: %s", startIndex, length, valueCount);
555-
final int startPoint = offsetBuffer.getInt(startIndex * OFFSET_WIDTH);
556-
final int sliceLength = offsetBuffer.getInt((startIndex + length) * OFFSET_WIDTH) - startPoint;
557555
to.clear();
558-
to.offsetBuffer = to.allocateOffsetBuffer((length + 1) * OFFSET_WIDTH);
559-
/* splitAndTransfer offset buffer */
560-
for (int i = 0; i < length + 1; i++) {
561-
final int relativeOffset = offsetBuffer.getInt((startIndex + i) * OFFSET_WIDTH) - startPoint;
562-
to.offsetBuffer.setInt(i * OFFSET_WIDTH, relativeOffset);
556+
if (length > 0) {
557+
final int startPoint = offsetBuffer.getInt(startIndex * OFFSET_WIDTH);
558+
final int sliceLength = offsetBuffer.getInt((startIndex + length) * OFFSET_WIDTH) - startPoint;
559+
to.offsetBuffer = to.allocateOffsetBuffer((length + 1) * OFFSET_WIDTH);
560+
/* splitAndTransfer offset buffer */
561+
for (int i = 0; i < length + 1; i++) {
562+
final int relativeOffset = offsetBuffer.getInt((startIndex + i) * OFFSET_WIDTH) - startPoint;
563+
to.offsetBuffer.setInt(i * OFFSET_WIDTH, relativeOffset);
564+
}
565+
/* splitAndTransfer validity buffer */
566+
splitAndTransferValidityBuffer(startIndex, length, to);
567+
/* splitAndTransfer data buffer */
568+
dataTransferPair.splitAndTransfer(startPoint, sliceLength);
569+
to.lastSet = length - 1;
570+
to.setValueCount(length);
563571
}
564-
/* splitAndTransfer validity buffer */
565-
splitAndTransferValidityBuffer(startIndex, length, to);
566-
/* splitAndTransfer data buffer */
567-
dataTransferPair.splitAndTransfer(startPoint, sliceLength);
568-
to.lastSet = length - 1;
569-
to.setValueCount(length);
570572
}
571573

572574
/*

‎java/vector/src/test/java/org/apache/arrow/vector/TestLargeVarCharVector.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ public void testInvalidStartIndex() {
166166
IllegalArgumentException.class,
167167
() -> tp.splitAndTransfer(valueCount, 10));
168168

169-
assertEquals("Invalid startIndex: 500", e.getMessage());
169+
assertEquals("Invalid parameters startIndex: 500, length: 10 for valueCount: 500", e.getMessage());
170170
}
171171
}
172172

@@ -185,7 +185,7 @@ public void testInvalidLength() {
185185
IllegalArgumentException.class,
186186
() -> tp.splitAndTransfer(0, valueCount * 2));
187187

188-
assertEquals("Invalid length: 1000", e.getMessage());
188+
assertEquals("Invalid parameters startIndex: 0, length: 1000 for valueCount: 500", e.getMessage());
189189
}
190190
}
191191

0 commit comments

Comments
 (0)