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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Support nested records in the Arrow Builder #1429

Merged
merged 15 commits into from Mar 11, 2021
Merged

Support nested records in the Arrow Builder #1429

merged 15 commits into from Mar 11, 2021

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Mar 9, 2021 •
edited

馃摂 Description

This adds support for records to the Arrow table slice builder.

馃摑 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

馃幆 Review Instructions

File-by-file.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Mar 9, 2021
@dominiklohmann dominiklohmann requested a review from a team March 9, 2021 13:01
mavam
mavam requested changes Mar 9, 2021
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

I would like to see a brief unit and/or integration test before greenlighting this PR.

CHANGELOG.md Outdated Show resolved Hide resolved
libvast/src/arrow_table_slice_builder.cpp Outdated Show resolved Hide resolved
libvast/src/arrow_table_slice_builder.cpp Outdated Show resolved Hide resolved
@dominiklohmann
Copy link
Member Author

I would like to see a brief unit and/or integration test before greenlighting this PR.

Working on that currently. I'm trying to look into the feasibility of adding the missing conversions to the JSON reader so an integration test becomes possible. Then the second sentence from the changelog entry becomes irrelevant as well.

@dominiklohmann dominiklohmann force-pushed the story/ch23298/arrow-nested-records branch 2 times, most recently from 364603c to a6986de Compare March 9, 2021 15:25
mavam
mavam reviewed Mar 9, 2021
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Why is the second baseline empty? Shouldn't it return the ingested data?

Since we're not querying from the nested record, I thought this would work.

libvast/src/format/json.cpp Show resolved Hide resolved
@dominiklohmann
Copy link
Member Author

dominiklohmann commented Mar 10, 2021 •
edited

Why is the second baseline empty? Shouldn't it return the ingested data?

Because it is non-functional at this point. I posted about the issue in Slack and would like to evaluate this with either @tobim or you sometime today鈥攁 second pair of eyes may help here.

Edit: Fixed.

tobim
tobim reviewed Mar 10, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@dominiklohmann dominiklohmann force-pushed the story/ch23298/arrow-nested-records branch from c10eae0 to 05ad4bc Compare March 10, 2021 17:14
mavam
mavam approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Nice work. This is perfect prep for the upcoming nested lists change.

I didn't test this locally but verified the integration test baseline matches my expectation.

vast/integration/reference/nested-records/step_01.ref Show resolved Hide resolved
libvast/src/index/list_index.cpp Outdated Show resolved Hide resolved
dominiklohmann
dominiklohmann commented Mar 10, 2021
View reviewed changes
libvast/src/index/list_index.cpp Show resolved Hide resolved
libvast/src/arrow_table_slice_builder.cpp Show resolved Hide resolved
libvast/src/arrow_table_slice_builder.cpp Outdated Show resolved Hide resolved
libvast/src/arrow_table_slice.cpp Outdated Show resolved Hide resolved
@dominiklohmann dominiklohmann merged commit 54549b8 into master Mar 11, 2021
@dominiklohmann dominiklohmann deleted the story/ch23298/arrow-nested-records branch March 11, 2021 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavam mavam mavam approved these changes

@tobim tobim Awaiting requested review from tobim

Assignees
No one assigned
Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
3 participants
@dominiklohmann @mavam @tobim