Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-16754: [Java] StructVector's child vectors get unexpectedly reordered after adding duplicated fields #13321

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

zhztheplayer
Copy link
Member

No description provided.

@github-actions
Copy link

github-actions bot commented Jun 6, 2022

@zhztheplayer zhztheplayer force-pushed the ARROW-16754 branch 2 times, most recently from 0618667 to 7258fd1 Compare June 6, 2022 16:24
@pitrou
Copy link
Member

pitrou commented Jun 7, 2022

@lidavidm @davisusanibar Would one of you want to review this?

// in the struct vector
initFields.add(Field.nullable("varchar1", MinorType.VARCHAR.getType()));
initFields.add(Field.nullable("varchar2", MinorType.VARCHAR.getType()));
initFields.add(Field.nullable("varchar3", MinorType.VARCHAR.getType()));
Copy link
Member

Choose a reason for hiding this comment

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

Should we also test a duplicated field name with a different type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

LGTM overall, with same coment as Antoine.

Should we also test the behavior with VectorSchemaRoot and unions? Presumably the same problem applied to them too.

@lidavidm
Copy link
Member

CC @davisusanibar if you'd like to carry this over the finish line

@zhztheplayer
Copy link
Member Author

I've addressed the last comment @pitrou @lidavidm please let me know if other changes are needed.

@lidavidm lidavidm merged commit 8c6d326 into apache:master Aug 19, 2022
@ursabot
Copy link

ursabot commented Aug 19, 2022

Benchmark runs are scheduled for baseline = b11bc50 and contender = 8c6d326. 8c6d326 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.69% ⬆️0.07%] test-mac-arm
[Failed ⬇️0.55% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.64% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 8c6d3263 ec2-t3-xlarge-us-east-2
[Failed] 8c6d3263 test-mac-arm
[Failed] 8c6d3263 ursa-i9-9960x
[Finished] 8c6d3263 ursa-thinkcentre-m75q
[Finished] b11bc505 ec2-t3-xlarge-us-east-2
[Failed] b11bc505 test-mac-arm
[Failed] b11bc505 ursa-i9-9960x
[Finished] b11bc505 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Aug 19, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…rdered after adding duplicated fields (apache#13321)

Authored-by: Hongze Zhang <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants