Skip to content

ARROW-4539: [Java] Fix child vector count for lists. - #3625

Merged
siddharthteotia merged 2 commits into
apache:masterfrom
praveenbingo:ARROW-4539
Feb 13, 2019
Merged

ARROW-4539: [Java] Fix child vector count for lists.#3625
siddharthteotia merged 2 commits into
apache:masterfrom
praveenbingo:ARROW-4539

Conversation

@praveenbingo

Copy link
Copy Markdown
Contributor
  • Child vector count was not set correctly for lists. Fixed to use the right count.

- Child vector count was not set correctly for lists. Fixed to use the right count.
@praveenbingo

Copy link
Copy Markdown
ContributorAuthor

@siddharthteotia - could you please review..

@@ -0,0 +1,65 @@
package org.apache.arrow.vector;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This files needs an Apache license header.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xhochy yup missed it..added them now.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #3625 into master will increase coverage by 1.23%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #3625 +/- ##
=========================================
+ Coverage 87.76% 89% +1.23% 
=========================================
Files 673 447 -226 Lines 82839 53792 -29047 Branches 1069 0 -1069 =========================================
- Hits 72704 47877 -24827 + Misses 10020 5915 -4105 + Partials 115 0 -115
Impacted FilesCoverage Δ
cpp/src/arrow/csv/reader.h0% <0%> (-100%)⬇️
cpp/src/arrow/array/builder_union.h0% <0%> (-100%)⬇️
cpp/src/arrow/array/builder_union.cc4.16% <0%> (-95.84%)⬇️
cpp/src/arrow/csv/reader.cc0.53% <0%> (-90.38%)⬇️
cpp/src/arrow/adapters/orc/adapter.cc0.26% <0%> (-73.04%)⬇️
cpp/src/arrow/csv/options.h66.66% <0%> (-33.34%)⬇️
cpp/src/parquet/column_scanner.cc59.09% <0%> (-22.73%)⬇️
cpp/src/arrow/vendored/xxhash/xxhash.c51.26% <0%> (-22.34%)⬇️
cpp/src/arrow/memory_pool.cc68.8% <0%> (-16.52%)⬇️
cpp/src/arrow/io/interfaces.cc40% <0%> (-15%)⬇️
... and 277 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c279eb...09bf45c. Read the comment docs.

/* valueCount for the data vector is the current end offset */
final int childValueCount = (valueCount == 0) ? 0 :
offsetBuffer.getInt(valueCount * OFFSET_WIDTH);
offsetBuffer.getInt(lastSet * OFFSET_WIDTH);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So were we always setting the value count for child vector incorrectly? I am not clear what the problem is. Is there a special case we have run into?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I ran into this over on #3294, when I tried to convert a string array from a JDBC field into a VarChar ListVector. If one of the values in the JDBC string array was null, I couldn't read the value after it from the VarChar ListVector (the following value came back as an empty string).

@praveenbingo was kind enough to put together this PR to fix the issue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, @siddharthteotia - we were not correctly handling lists that could have null values (setting child vector count using valuecount will not handle cases where valuecount < lastSet i.e. presence of nulls).

It does not affect List of FixedWidthVectors and only VariableWidthVectors.

Please let me know if you would need more information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddharthteotiasiddharthteotia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@siddharthteotia
siddharthteotia merged commit 51b5143 into apache:masterFeb 13, 2019
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
* ARROW-4539: [Java] Fix child vector count for lists.
- Child vector count was not set correctly for lists. Fixed to use the right count.
* ARROW-4539: [Java] Add license header.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@praveenbingo@codecov-io@xhochy@siddharthteotia@mikepigott