-
Notifications
You must be signed in to change notification settings - Fork 41
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
fix: add signature for read_excel #2376
Conversation
…b.com:GlareDB/glaredb into universalmind303/excel-signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick Q:
In the PR description, why is there two Utf8's in a row?
[Utf8, Utf8, sheet_name: Utf8, infer_rows: UInt64, has_header: Boolean]
The first seems to be for required path arg, but what's the second?
let options: Fields = vec![ | ||
Field::new("sheet_name", DataType::Utf8, true), | ||
Field::new("infer_rows", DataType::UInt64, true), | ||
Field::new("has_header", DataType::Boolean, true), | ||
] | ||
.into_iter() | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From is implemented for Vec<Field>
and Fields, so I think FromIterator handlers, which might make this simpler?
To be clear, this is fine, but I definitely stopped and read this three times because it looks wrong. "why are we making a vector, converting it to an iterator and collecting it back into a vector without doing anything else?" and while it compiles and does the thing, mds
If it's easy enough to change I think that would make it better and less likely to confuse other people in the future.
Datafusion doesn't really give us that good formatting out of the box for the signatures. so the first signature is @greyscaled I agree that the formatting choices are not great though. |
all good, I appreciate the explanation |
closes #2373