Add array indexing support to Parquet variant field extraction - #22895
Conversation
vuule
added
feature request
vuule
changed the title
| if (variant_basic_type(value_metadata) != basic_type::array) { return {}; } | ||
|
|
||
| int const value_header = variant_value_header(value_metadata); | ||
| [[maybe_unused]] auto const [offset_size, id_size, num_elements_size] = |
There was a problem hiding this comment.
Is id_size used anywhere? I think in C++ 26 you can use the underscore to indicate unused variables, but I'm unsure what standard we use.
There was a problem hiding this comment.
We currently build with C++20. [[maybe_unused]] is applied to all bindings here. I also renamed id_size to _ to make it clearer which binding is unused.
|
/ok to test fdd70bb |
📝 WalkthroughWalkthroughAdds bracketed array-index steps to VARIANT paths, validates malformed index syntax, resolves indexed Apache VARIANT arrays with bounds checks, and expands C++ and Java test coverage for nested traversal and malformed inputs. ChangesVariant array path support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/io/experimental/variant_extract_test.cpp (1)
514-637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a sliced-column case for array-index paths.
The new array-indexing tests (
ApacheArrayPrimitiveIndexing,EmptyArrayIndexing,MixedObjectArrayTraversal, etc.) all use unsliced columns.get_variant_fieldresolves metadata/value spans viaget_sliced_child, so slice-offset arithmetic combined with array-index resolution is untested by this PR.As per coding guidelines, "Tests must cover empty inputs, nulls, sliced columns, boundary and multi-block sizes, and non-ASCII UTF-8 for string tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/io/experimental/variant_extract_test.cpp` around lines 514 - 637, Add sliced-column coverage to the array-index path tests, preferably in ApacheArrayPrimitiveIndexing or MixedObjectArrayTraversal, by creating a sliced variant column with a nonzero offset and asserting representative indexed paths still return the expected values and nulls. Reuse the existing extraction helpers and fixtures while ensuring the slice exercises get_sliced_child metadata/value span arithmetic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tests/io/experimental/variant_extract_test.cpp`:
- Around line 514-637: Add sliced-column coverage to the array-index path tests,
preferably in ApacheArrayPrimitiveIndexing or MixedObjectArrayTraversal, by
creating a sliced variant column with a nonzero offset and asserting
representative indexed paths still return the expected values and nulls. Reuse
the existing extraction helpers and fixtures while ensuring the slice exercises
get_sliced_child metadata/value span arithmetic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: afccee14-ba8d-4397-a369-a235ec56ff66
📒 Files selected for processing (6)
cpp/include/cudf/io/experimental/variant.hppcpp/src/io/parquet/experimental/variant_extract.cucpp/src/io/parquet/experimental/variant_path.cppcpp/src/io/parquet/experimental/variant_path.hppcpp/tests/io/experimental/variant_extract_test.cppjava/src/test/java/ai/rapids/cudf/VariantUtilsTest.java
Extend the JSONPath-like path used by get_variant_field/extract_variant_field
to descend into array values via zero-based "[N]" steps, in addition to the
existing object-key descent.
- variant_path.cpp: parse "[<non-negative integer>]" steps, preserving the
literal "[N]" token so the GPU path walker can distinguish index steps from
object-key steps by their first byte. Wildcards ("[*]"), negative indices,
quoted names in brackets, and unterminated brackets throw.
- variant_extract.cu: add locate_array_element (parses the array value header
and returns the N-th element's subspan) and dispatch on "[" in resolve_path.
- variant.hpp: document the extended path grammar and array-indexing behavior.
- Tests: positive int8 array indexing, out-of-bounds / type-mismatch nulls, and
updated SyntaxErrors to accept "$.a[0]" while rejecting "$.a[-1]" and "$.a[1".
Verified locally: VARIANT_EXTRACT_TEST 33/33 pass.
Reject out-of-range "[N]" indices at parse time (std::from_chars bound to cudf::size_type) and accumulate the index in uint64_t on the GPU path walker to avoid signed-int32 overflow (UB). Add a SyntaxErrors case for an over-long index, document that out-of-range indices throw, and note that array element offsets are monotonic (unlike object field offsets).
fdd70bb to
9d84fe3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cudf_polars/pyproject.toml (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPer-test timeout protection was dropped
python/cudf_polars/pyproject.toml#L48-L50,93-L94and the CI runners now rely only onci/timeout_with_stack.pyfor a 3600s whole-run timeout. That means one hung test can stall the entire suite for up to an hour instead of failing independently. Keep a per-test timeout guard, or explicitly accept the loss of suite continuation on hangs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/pyproject.toml` at line 1, Restore per-test timeout protection in the pytest configuration within pyproject.toml, alongside the existing suite-level ci/timeout_with_stack.py guard. Configure the timeout plugin or equivalent test-level mechanism so each hung test fails independently while preserving the current whole-run timeout.java/ci/build_static_libcudf_in_container.sh (1)
91-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a trap to chown
/outputon any exit, not only on success.If
cmake --installfails partway through, files already written to/output(bind-mounted to the hostOUTPUT_DIR) stay root-owned since the unconditional chown at the end never runs. The siblingbuild_cudf_java_jar_in_container.shalready solves the identical problem viatrap _chown_outputs_on_exit EXIT; this script should do the same for consistency and to avoid leaving root-owned partial output that a subsequent local re-run'srm -rf(seetest_java_build_local.shlines 154-157) may struggle to clean up.🔧 Proposed fix
+_chown_output_on_exit() { + chown -R "${HOST_UID}:${HOST_GID}" "${INSTALL_PREFIX}" 2>/dev/null || true +} +trap _chown_output_on_exit EXIT + rapids-logger "Installing static libcudf to ${INSTALL_PREFIX}" cmake --install "${BUILD_DIR}" - -rapids-logger "Chowning ${INSTALL_PREFIX} to ${HOST_UID}:${HOST_GID}" -chown -R "${HOST_UID}:${HOST_GID}" "${INSTALL_PREFIX}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/ci/build_static_libcudf_in_container.sh` around lines 91 - 96, Add an EXIT trap in build_static_libcudf_in_container.sh matching the sibling script’s _chown_outputs_on_exit pattern, ensuring /output is chowned to HOST_UID:HOST_GID on both successful and failed exits. Remove or avoid relying on the later unconditional chown after cmake --install so partial installations are also repaired.
🧹 Nitpick comments (2)
ci/timeout_with_stack.py (1)
210-240: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
psutil.wait_procsinstead of sequential per-child waits.Waiting on each child one at a time (lines 222-225) makes the worst-case wait time scale with the number of children (
num_children × 3s), adding avoidable delay to an already-timed-out run (e.g.-n 4pytest-xdist workers).psutil.wait_procs()waits on the whole list concurrently against one shared deadline and returns the still-alive subset directly.♻️ Proposed refactor using `psutil.wait_procs`
try: parent = psutil.Process(pid) children = parent.children(recursive=True) # Terminate children first for child in children: with suppress(psutil.NoSuchProcess): child.terminate() - # Create a copy of children list - terminated_children = list(children) - - # Wait for all children to terminate - for child in terminated_children: - with suppress(psutil.TimeoutExpired): - child.wait(timeout=3) - - # Kill any remaining children - for child in terminated_children: - with suppress(psutil.NoSuchProcess): - child.kill() + # Wait on all children concurrently, then kill any stragglers + _, alive = psutil.wait_procs(children, timeout=3) + for child in alive: + with suppress(psutil.NoSuchProcess): + child.kill() # Terminate parent parent.terminate()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/timeout_with_stack.py` around lines 210 - 240, Replace the sequential child.wait loop in the process-termination flow with psutil.wait_procs using the terminated_children list and a single 3-second timeout. Use its returned alive subset when killing remaining children, while preserving the existing parent termination and NoSuchProcess handling.java/ci/README.md (1)
47-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language hint to the fenced directory-tree blocks.
markdownlint (MD040) flags the fenced blocks at Line 47 and Line 75 for missing a language identifier.
📝 Proposed fix
-``` +```text /tmp/jars/cuda12/ cudf-26.08.0-SNAPSHOT-cuda12.jar cudf-26.08.0-SNAPSHOT.pom-``` +```text /tmp/maven-repo/ai/rapids/cudf/26.08.0-SNAPSHOT/ cudf-26.08.0-SNAPSHOT-cuda12.jar cudf-26.08.0-SNAPSHOT-cuda13.jar cudf-26.08.0-SNAPSHOT.pom🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/ci/README.md` around lines 47 - 75, Add the text language identifier to both fenced directory-tree blocks in the README, including the examples beginning with /tmp/jars/cuda12/ and /tmp/maven-repo/ai/rapids/cudf/. Preserve their displayed tree contents unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@java/ci/build_static_libcudf_in_container.sh`:
- Line 19: Enable pipefail in both scripts before their
rapids-dependency-file-generator pipelines: update
java/ci/build_static_libcudf_in_container.sh lines 19-19 and
java/ci/build_cudf_java_jar_in_container.sh lines 22-22 to use set -eo pipefail
or an equivalent separate setting. Ensure generator failures propagate instead
of being masked by tee.
In `@java/ci/build_static_libcudf.sh`:
- Around line 103-104: Update the output-directory setup in
build_static_libcudf.sh to reject a pre-existing non-empty --output-dir before
creating or resolving it, matching the stale-output protection used by
CLASSIFIER_OUT in build_cudf_java_jar.sh. Preserve support for a new or empty
directory and only continue with the build after validating it is safe to reuse.
---
Outside diff comments:
In `@java/ci/build_static_libcudf_in_container.sh`:
- Around line 91-96: Add an EXIT trap in build_static_libcudf_in_container.sh
matching the sibling script’s _chown_outputs_on_exit pattern, ensuring /output
is chowned to HOST_UID:HOST_GID on both successful and failed exits. Remove or
avoid relying on the later unconditional chown after cmake --install so partial
installations are also repaired.
In `@python/cudf_polars/pyproject.toml`:
- Line 1: Restore per-test timeout protection in the pytest configuration within
pyproject.toml, alongside the existing suite-level ci/timeout_with_stack.py
guard. Configure the timeout plugin or equivalent test-level mechanism so each
hung test fails independently while preserving the current whole-run timeout.
---
Nitpick comments:
In `@ci/timeout_with_stack.py`:
- Around line 210-240: Replace the sequential child.wait loop in the
process-termination flow with psutil.wait_procs using the terminated_children
list and a single 3-second timeout. Use its returned alive subset when killing
remaining children, while preserving the existing parent termination and
NoSuchProcess handling.
In `@java/ci/README.md`:
- Around line 47-75: Add the text language identifier to both fenced
directory-tree blocks in the README, including the examples beginning with
/tmp/jars/cuda12/ and /tmp/maven-repo/ai/rapids/cudf/. Preserve their displayed
tree contents unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f39df46d-ff2d-4d85-b5d4-2f0b0c648b6d
📒 Files selected for processing (37)
.github/workflows/build.yamlci/run_cudf_polars_polars_tests.shci/run_cudf_polars_pytests.shci/test_wheel_cudf_polars.shci/timeout_with_stack.pyconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlcpp/include/cudf/io/experimental/variant.hppcpp/src/io/parquet/experimental/variant_extract.cucpp/src/io/parquet/experimental/variant_path.cppcpp/src/io/parquet/experimental/variant_path.hppcpp/tests/io/experimental/variant_extract_test.cppdependencies.yamljava/ci/README.mdjava/ci/argparse.shjava/ci/assemble_maven_repo.shjava/ci/build_cudf_java_jar.shjava/ci/build_cudf_java_jar_in_container.shjava/ci/build_static_libcudf.shjava/ci/build_static_libcudf_in_container.shjava/ci/test_java_build_local.shjava/pom.xmljava/src/test/java/ai/rapids/cudf/VariantUtilsTest.javapython/cudf/cudf/core/column/numerical.pypython/cudf/cudf/pandas/_wrappers/pandas.pypython/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/series/methods/test_astype.pypython/cudf/cudf_pandas_tests/test_cudf_pandas.pypython/cudf/cudf_pandas_tests/test_fast_slow_proxy.pypython/cudf_polars/pyproject.tomlpython/cudf_polars/tests/conftest.pypython/cudf_polars/tests/expressions/test_rolling.pypython/cudf_polars/tests/streaming/test_scan.pypython/cudf_polars/tests/streaming/test_sort.py
💤 Files with no reviewable changes (4)
- python/cudf_polars/tests/conftest.py
- python/cudf_polars/tests/streaming/test_sort.py
- python/cudf_polars/tests/streaming/test_scan.py
- python/cudf_polars/tests/expressions/test_rolling.py
🚧 Files skipped from review as they are similar to previous changes (6)
- cpp/src/io/parquet/experimental/variant_path.hpp
- java/src/test/java/ai/rapids/cudf/VariantUtilsTest.java
- cpp/include/cudf/io/experimental/variant.hpp
- cpp/src/io/parquet/experimental/variant_extract.cu
- cpp/tests/io/experimental/variant_extract_test.cpp
- cpp/src/io/parquet/experimental/variant_path.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cudf_polars/pyproject.toml (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPer-test timeout protection was dropped
python/cudf_polars/pyproject.toml#L48-L50,93-L94and the CI runners now rely only onci/timeout_with_stack.pyfor a 3600s whole-run timeout. That means one hung test can stall the entire suite for up to an hour instead of failing independently. Keep a per-test timeout guard, or explicitly accept the loss of suite continuation on hangs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/pyproject.toml` at line 1, Restore per-test timeout protection in the pytest configuration within pyproject.toml, alongside the existing suite-level ci/timeout_with_stack.py guard. Configure the timeout plugin or equivalent test-level mechanism so each hung test fails independently while preserving the current whole-run timeout.java/ci/build_static_libcudf_in_container.sh (1)
91-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a trap to chown
/outputon any exit, not only on success.If
cmake --installfails partway through, files already written to/output(bind-mounted to the hostOUTPUT_DIR) stay root-owned since the unconditional chown at the end never runs. The siblingbuild_cudf_java_jar_in_container.shalready solves the identical problem viatrap _chown_outputs_on_exit EXIT; this script should do the same for consistency and to avoid leaving root-owned partial output that a subsequent local re-run'srm -rf(seetest_java_build_local.shlines 154-157) may struggle to clean up.🔧 Proposed fix
+_chown_output_on_exit() { + chown -R "${HOST_UID}:${HOST_GID}" "${INSTALL_PREFIX}" 2>/dev/null || true +} +trap _chown_output_on_exit EXIT + rapids-logger "Installing static libcudf to ${INSTALL_PREFIX}" cmake --install "${BUILD_DIR}" - -rapids-logger "Chowning ${INSTALL_PREFIX} to ${HOST_UID}:${HOST_GID}" -chown -R "${HOST_UID}:${HOST_GID}" "${INSTALL_PREFIX}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/ci/build_static_libcudf_in_container.sh` around lines 91 - 96, Add an EXIT trap in build_static_libcudf_in_container.sh matching the sibling script’s _chown_outputs_on_exit pattern, ensuring /output is chowned to HOST_UID:HOST_GID on both successful and failed exits. Remove or avoid relying on the later unconditional chown after cmake --install so partial installations are also repaired.
🧹 Nitpick comments (2)
ci/timeout_with_stack.py (1)
210-240: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
psutil.wait_procsinstead of sequential per-child waits.Waiting on each child one at a time (lines 222-225) makes the worst-case wait time scale with the number of children (
num_children × 3s), adding avoidable delay to an already-timed-out run (e.g.-n 4pytest-xdist workers).psutil.wait_procs()waits on the whole list concurrently against one shared deadline and returns the still-alive subset directly.♻️ Proposed refactor using `psutil.wait_procs`
try: parent = psutil.Process(pid) children = parent.children(recursive=True) # Terminate children first for child in children: with suppress(psutil.NoSuchProcess): child.terminate() - # Create a copy of children list - terminated_children = list(children) - - # Wait for all children to terminate - for child in terminated_children: - with suppress(psutil.TimeoutExpired): - child.wait(timeout=3) - - # Kill any remaining children - for child in terminated_children: - with suppress(psutil.NoSuchProcess): - child.kill() + # Wait on all children concurrently, then kill any stragglers + _, alive = psutil.wait_procs(children, timeout=3) + for child in alive: + with suppress(psutil.NoSuchProcess): + child.kill() # Terminate parent parent.terminate()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/timeout_with_stack.py` around lines 210 - 240, Replace the sequential child.wait loop in the process-termination flow with psutil.wait_procs using the terminated_children list and a single 3-second timeout. Use its returned alive subset when killing remaining children, while preserving the existing parent termination and NoSuchProcess handling.java/ci/README.md (1)
47-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language hint to the fenced directory-tree blocks.
markdownlint (MD040) flags the fenced blocks at Line 47 and Line 75 for missing a language identifier.
📝 Proposed fix
-``` +```text /tmp/jars/cuda12/ cudf-26.08.0-SNAPSHOT-cuda12.jar cudf-26.08.0-SNAPSHOT.pom-``` +```text /tmp/maven-repo/ai/rapids/cudf/26.08.0-SNAPSHOT/ cudf-26.08.0-SNAPSHOT-cuda12.jar cudf-26.08.0-SNAPSHOT-cuda13.jar cudf-26.08.0-SNAPSHOT.pom🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/ci/README.md` around lines 47 - 75, Add the text language identifier to both fenced directory-tree blocks in the README, including the examples beginning with /tmp/jars/cuda12/ and /tmp/maven-repo/ai/rapids/cudf/. Preserve their displayed tree contents unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@java/ci/build_static_libcudf_in_container.sh`:
- Line 19: Enable pipefail in both scripts before their
rapids-dependency-file-generator pipelines: update
java/ci/build_static_libcudf_in_container.sh lines 19-19 and
java/ci/build_cudf_java_jar_in_container.sh lines 22-22 to use set -eo pipefail
or an equivalent separate setting. Ensure generator failures propagate instead
of being masked by tee.
In `@java/ci/build_static_libcudf.sh`:
- Around line 103-104: Update the output-directory setup in
build_static_libcudf.sh to reject a pre-existing non-empty --output-dir before
creating or resolving it, matching the stale-output protection used by
CLASSIFIER_OUT in build_cudf_java_jar.sh. Preserve support for a new or empty
directory and only continue with the build after validating it is safe to reuse.
---
Outside diff comments:
In `@java/ci/build_static_libcudf_in_container.sh`:
- Around line 91-96: Add an EXIT trap in build_static_libcudf_in_container.sh
matching the sibling script’s _chown_outputs_on_exit pattern, ensuring /output
is chowned to HOST_UID:HOST_GID on both successful and failed exits. Remove or
avoid relying on the later unconditional chown after cmake --install so partial
installations are also repaired.
In `@python/cudf_polars/pyproject.toml`:
- Line 1: Restore per-test timeout protection in the pytest configuration within
pyproject.toml, alongside the existing suite-level ci/timeout_with_stack.py
guard. Configure the timeout plugin or equivalent test-level mechanism so each
hung test fails independently while preserving the current whole-run timeout.
---
Nitpick comments:
In `@ci/timeout_with_stack.py`:
- Around line 210-240: Replace the sequential child.wait loop in the
process-termination flow with psutil.wait_procs using the terminated_children
list and a single 3-second timeout. Use its returned alive subset when killing
remaining children, while preserving the existing parent termination and
NoSuchProcess handling.
In `@java/ci/README.md`:
- Around line 47-75: Add the text language identifier to both fenced
directory-tree blocks in the README, including the examples beginning with
/tmp/jars/cuda12/ and /tmp/maven-repo/ai/rapids/cudf/. Preserve their displayed
tree contents unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f39df46d-ff2d-4d85-b5d4-2f0b0c648b6d
📒 Files selected for processing (37)
.github/workflows/build.yamlci/run_cudf_polars_polars_tests.shci/run_cudf_polars_pytests.shci/test_wheel_cudf_polars.shci/timeout_with_stack.pyconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlcpp/include/cudf/io/experimental/variant.hppcpp/src/io/parquet/experimental/variant_extract.cucpp/src/io/parquet/experimental/variant_path.cppcpp/src/io/parquet/experimental/variant_path.hppcpp/tests/io/experimental/variant_extract_test.cppdependencies.yamljava/ci/README.mdjava/ci/argparse.shjava/ci/assemble_maven_repo.shjava/ci/build_cudf_java_jar.shjava/ci/build_cudf_java_jar_in_container.shjava/ci/build_static_libcudf.shjava/ci/build_static_libcudf_in_container.shjava/ci/test_java_build_local.shjava/pom.xmljava/src/test/java/ai/rapids/cudf/VariantUtilsTest.javapython/cudf/cudf/core/column/numerical.pypython/cudf/cudf/pandas/_wrappers/pandas.pypython/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/series/methods/test_astype.pypython/cudf/cudf_pandas_tests/test_cudf_pandas.pypython/cudf/cudf_pandas_tests/test_fast_slow_proxy.pypython/cudf_polars/pyproject.tomlpython/cudf_polars/tests/conftest.pypython/cudf_polars/tests/expressions/test_rolling.pypython/cudf_polars/tests/streaming/test_scan.pypython/cudf_polars/tests/streaming/test_sort.py
💤 Files with no reviewable changes (4)
- python/cudf_polars/tests/conftest.py
- python/cudf_polars/tests/streaming/test_sort.py
- python/cudf_polars/tests/streaming/test_scan.py
- python/cudf_polars/tests/expressions/test_rolling.py
🚧 Files skipped from review as they are similar to previous changes (6)
- cpp/src/io/parquet/experimental/variant_path.hpp
- java/src/test/java/ai/rapids/cudf/VariantUtilsTest.java
- cpp/include/cudf/io/experimental/variant.hpp
- cpp/src/io/parquet/experimental/variant_extract.cu
- cpp/tests/io/experimental/variant_extract_test.cpp
- cpp/src/io/parquet/experimental/variant_path.cpp
🛑 Comments failed to post (2)
java/ci/build_static_libcudf_in_container.sh (1)
19-19: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add
set -o pipefailto both in-container scripts to stop maskingrapids-dependency-file-generatorfailures.Both scripts pipe
rapids-dependency-file-generator ... | tee "${ENV_YAML_DIR}/env.yaml"underset -ewithoutpipefail. If the generator fails,tee's own success makes the pipeline's exit status 0, so the script continues with a possibly empty/malformedenv.yamlstraight intorapids-mamba-retry env create, turning a clear upstream failure into a confusing downstream one.
java/ci/build_static_libcudf_in_container.sh#L19: changeset -etoset -eo pipefail(or add a separateset -o pipefailline) before therapids-dependency-file-generator | teecall at Line 48-51.java/ci/build_cudf_java_jar_in_container.sh#L22: apply the sameset -eo pipefailchange before the equivalentrapids-dependency-file-generator | teecall at Line 56-59.🔧 Proposed fix (apply to both files)
-set -e +set -eo pipefail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.set -eo pipefail📍 Affects 2 files
java/ci/build_static_libcudf_in_container.sh#L19-L19(this comment)java/ci/build_cudf_java_jar_in_container.sh#L22-L22🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/ci/build_static_libcudf_in_container.sh` at line 19, Enable pipefail in both scripts before their rapids-dependency-file-generator pipelines: update java/ci/build_static_libcudf_in_container.sh lines 19-19 and java/ci/build_cudf_java_jar_in_container.sh lines 22-22 to use set -eo pipefail or an equivalent separate setting. Ensure generator failures propagate instead of being masked by tee.java/ci/build_static_libcudf.sh (1)
103-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard against reusing a stale
--output-dir.Unlike
build_cudf_java_jar.sh'sCLASSIFIER_OUTcheck, this script silently reuses whatever already exists at--output-dir. A leftover install tree from a prior/failed run or a different CUDA version could mix with the new build and get linked against downstream inbuild_cudf_java_jar_in_container.sh, producing a subtly wronglibcudfjni.so.🛡️ Proposed fix
mkdir -p "${OUTPUT_DIR}" +if [[ -n "$(ls -A "${OUTPUT_DIR}" 2>/dev/null)" ]]; then + echo "Error: --output-dir '${OUTPUT_DIR}' must be empty or nonexistent" >&2 + exit 1 +fi OUTPUT_DIR="$(cd "${OUTPUT_DIR}" && pwd)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.mkdir -p "${OUTPUT_DIR}" if [[ -n "$(ls -A "${OUTPUT_DIR}" 2>/dev/null)" ]]; then echo "Error: --output-dir '${OUTPUT_DIR}' must be empty or nonexistent" >&2 exit 1 fi OUTPUT_DIR="$(cd "${OUTPUT_DIR}" && pwd)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/ci/build_static_libcudf.sh` around lines 103 - 104, Update the output-directory setup in build_static_libcudf.sh to reject a pre-existing non-empty --output-dir before creating or resolving it, matching the stale-output protection used by CLASSIFIER_OUT in build_cudf_java_jar.sh. Preserve support for a new or empty directory and only continue with the build after validating it is safe to reuse.
PointKernel
left a comment
PointKernel
left a comment
There was a problem hiding this comment.
Two small comments but not blocking.
simoneves
left a comment
simoneves
left a comment
There was a problem hiding this comment.
Some questions, now that you've got the right Simon...
| auto const offsets_start = pos; | ||
| auto const offsets_bytes = (static_cast<uint64_t>(num_entries.value()) + 1) * offset_size; | ||
| if (offsets_bytes > static_cast<uint64_t>(meta_len - offsets_start)) { | ||
| if (cuda::std::cmp_greater(offsets_bytes, meta_len - offsets_start)) { |
There was a problem hiding this comment.
What is the benefit of using cuda::std::cmp_greater here? Both values are surely unsigned.
There was a problem hiding this comment.
offsets_bytes is unsigned, and meta_len - offsets_start is signed.
It's a convention in cudf since C++20 adoption to prefer std::cmp_xyz to casts for mixed type comparison.
| // Consume the maximal run of decimal digits. | ||
| std::size_t n = 1; | ||
| while (n < tail.size() && tail[n] >= '0' && tail[n] <= '9') { | ||
| ++n; |
There was a problem hiding this comment.
Do we need to tolerate white-space inside the [] here? This would no-op if the first thing after the [ was a space, and std::from_chars does not tolerate white-space anyway, not that it would get that far.
There was a problem hiding this comment.
I guess it would fail with the same error state either way. I just just wondering if it SHOULD tolerate white-space, if (like I asked elsewhere) whatever is upstream providing these expressions cannot be trusted not to have inserted any.
There was a problem hiding this comment.
In Apache Spark, Spark’s Variant path parser disables whitespace skipping and only accepts digits between the brackets. So $ [0] and $ [01] are valid, but $ [ 0] and $[0 ] are invalid.
scala> spark.sql("""
| SELECT try_variant_get(
| parse_json('[10, 20]'),
| '$[ 0]',
| 'int'
| )
| """).show(false)
org.apache.spark.SparkRuntimeException: [INVALID_VARIANT_GET_PATH] The path `$[ 0]` is not a valid variant extraction path in ``try_variant_get``.
| if (variant_basic_type(value_metadata) != basic_type::array) { return {}; } | ||
|
|
||
| int const value_header = variant_value_header(value_metadata); | ||
| [[maybe_unused]] auto const [offset_size, _, num_elements_size] = |
There was a problem hiding this comment.
I assume the [[maybe_unused]] is just to tolerate and discard the _?
There was a problem hiding this comment.
That's right. Using _ doesn't mean anything to the compiler (yet); it's just there to make it clear to the reader which one isn't used.
There was a problem hiding this comment.
...and avoid a compile warning, presumably.
There was a problem hiding this comment.
I think there wasn't a warning in this case. This one is for humans only :)
|
|
||
| size_type const offsets_start = position; | ||
| auto const offsets_bytes = (static_cast<uint64_t>(num_elements) + 1) * offset_size; | ||
| if (cuda::std::cmp_greater(offsets_bytes, value_size - offsets_start)) { return {}; } |
There was a problem hiding this comment.
Another one of these. I'm guessing this is just an agreed standard?
| "$.a[*]", | ||
| "$.a[-1]", | ||
| "$.a[+1]", | ||
| "$.a[ 1]", |
There was a problem hiding this comment.
I guess this answers my earlier question. But what is upstream of this that guarantees that there won't be any white-space?
There was a problem hiding this comment.
I haven't gotten the requirement to allow white-space in indices, so I'd prefer to keep things simple until we have to support this.
@nartal1 can confirm is this is needed.
There was a problem hiding this comment.
Spark throws error if there are whitespaces within array indices. #22895 (comment)
| () -> VariantUtils.getVariantFieldValue(variant, "$.x[")); | ||
| assertThrows(CudfException.class, | ||
| () -> VariantUtils.extractVariantField(variant, "$.x[0]", DType.INT32)); | ||
| () -> VariantUtils.extractVariantField(variant, "$.x[", DType.INT32)); |
There was a problem hiding this comment.
Is the assumption that the C++ tests cover all the bases, and this is just a smoke test?
There was a problem hiding this comment.
I think that's the intent, but I'm not sure. @nartal1 ?
There was a problem hiding this comment.
Yes, that's correct. Maybe we can add a smoke test for $[0] but that can be done in the later PR when we add the JNI for this.
|
For 1,048,576 rows, accessing the last element/field:
So, as expected, accessing indexed arrays scales better because we have direct access to the elements. |
|
/ok to test f9c1c2d |
|
/merge |
f6d1081
into
rapidsai:release/26.08
7 participants
Summary
Contributes to #22312
Extends the JSONPath-like path used by
get_variant_field/extract_variant_fieldto descend into array values via zero-based[N]index steps, in addition to the existing object-key descent. This enables extraction of array elements from Parquet VARIANT columns (e.g.$.a[0],$[0].field).Leading zeroes are allowed, whitespace characters are not.
Checklist