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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle alias types properly for CSV #1534

Merged
merged 4 commits into from Apr 8, 2021

Conversation

dominiklohmann
Copy link
Member

馃摂 Description

This fixes a crash with nested aliases in the CSV reader. To reproduce, replace the definition of port in base.schema like this:

type proxy = count
type port = proxy

Then import CSV data, e.g.,

gunzip -c vast/integration/data/csv/argus-M57-10k-pkts.csv.gz | vast -N import csv

馃摑 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

Commit-by-commit.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Apr 8, 2021
@dominiklohmann dominiklohmann requested a review from a team April 8, 2021 11:21
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Ideally this could use a unit test to ensure we'll not regress on this. But clearly not crashing is better than crashing, so I'm also not opposed to merging it immediately.

@dominiklohmann
Copy link
Member Author

dominiklohmann commented Apr 8, 2021

Ideally this could use a unit test to ensure we'll not regress on this. But clearly not crashing is better than crashing, so I'm also not opposed to merging it immediately.

We have this open story about possibly consolidating the data conversions to a given type (ch23655), and I think it makes more sense to add tests in the scope of that story.

I'll still try and write a minimal unit test for this one, but I'm unsure how feasible it is. I've added a minimal test for the table slice change.

@dominiklohmann dominiklohmann merged commit 9de84f7 into master Apr 8, 2021
@dominiklohmann dominiklohmann deleted the story/ch22830/fix-crash-in-csv-reader branch April 8, 2021 12:39
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
2 participants