Skip to content

Fix vast.export.json.omit-nulls for nested records #2447

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

Merged

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Jul 20, 2022

The --omit-nulls option of the JSON export overeagerly removed entire nested records whose first fields was null, instead of just removing the nested field.

📝 Checklist

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

🎯 Review Instructions

The new unit and integration tests cover this extensively. The code change is rather simple to follow.

Let's increment the number of bugs caused by table slice column access using flat indices rather than offsets and move that up a bit on the roadmap 🙈

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Jul 20, 2022
@dominiklohmann dominiklohmann requested a review from a team July 20, 2022 13:45
The `--omit-nulls` option of the JSON export overeagerly removed entire nested
records whose first fields was null, instead of just removing the nested field.
@dominiklohmann dominiklohmann force-pushed the topic/json-omit-nulls-first-nested-element-null branch from ec2f515 to 31b7d49 Compare July 20, 2022 13:47
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

I carefully read the new unit test and it is enough to convince me that the function now behaves as expected.

@dominiklohmann dominiklohmann merged commit d48aa78 into master Jul 20, 2022
@dominiklohmann dominiklohmann deleted the topic/json-omit-nulls-first-nested-element-null branch July 20, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants