Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
alamb commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559815607 thank you ... testing now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
etseidl commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559809768 > Oh no -- I liked the `format_nullability` ! we need to be on a call, we're switching too fast 😅 I reverted the revert and then merged your patch. I think we're on the same page now 😄 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
alamb commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559794717 Oh no -- I liked the `format_nullability` ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
alamb commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559779795 I pushed a small commit with a regression test -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
alamb commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559768940 Here is a proposed change: - https://github.com/etseidl/arrow-rs/pull/3 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
alamb commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559752110 > No worries...in the meantime I'm going to consolidate nullable processing so changing the tokens won't be such a pain. I have played around with it and did some more research, and I would like to propose yet another thing: `non-null` rather than `nonnull` -- that at least seems to be somewhat standard. I'll make a PR with a proposal -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
etseidl commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559734432 > Thank you so much @etseidl > > I hate to bikeshed this, but I am thinking maybe we could do > > `Int64 not null` to align more with sql syntax (rather than invent some other random syntax). > > However that is pretty verbose. > > Let me play around and see what it would look like No worries...in the meantime I'm going to consolidate nullable processing so changing the tokens won't be such a pain. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
etseidl commented on code in PR #8890:
URL: https://github.com/apache/arrow-rs/pull/8890#discussion_r2547143920
##
arrow-schema/src/datatype_display.rs:
##
@@ -27,7 +27,7 @@ impl fmt::Display for DataType {
fn format_field(field: &crate::Field) -> String {
let name = field.name();
-let maybe_nullable = if field.is_nullable() { "nullable " } else {
"" };
+let maybe_nullable = if field.is_nullable() { "" } else { "nonnull
" };
Review Comment:
maybe this logic should be its own function as it's used elsewhere
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
alamb commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559401839 will check this out in a few moments -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
etseidl commented on code in PR #8890:
URL: https://github.com/apache/arrow-rs/pull/8890#discussion_r2546861657
##
arrow-schema/src/datatype_parse.rs:
##
@@ -547,11 +547,15 @@ impl<'a> Parser<'a> {
))
}
-/// return and consume if the next token is `Token::Nullable`
+/// consume the next token and return `false` if the field is `nonnull`.
fn parse_opt_nullable(&mut self) -> bool {
-self.tokenizer
-.next_if(|next| matches!(next, Ok(Token::Nullable)))
-.is_some()
+let tok = self
+.tokenizer
+.next_if(|next| matches!(next, Ok(Token::NonNull |
Token::Nullable)));
+match tok {
+Some(Ok(Token::NonNull)) => false,
+_ => true,
+}
Review Comment:
I retained the "nullable" token because there was a backwards compatibility
test that required it. I'm not familiar with the old display behavior, so if
nothing used to emit "nullable" then this can be simplified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix regression caused by changes in Display for DataType [arrow-rs]
etseidl commented on PR #8890: URL: https://github.com/apache/arrow-rs/pull/8890#issuecomment-3559077232 @alamb here's my second attempt. Who else should take a look at this one? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] Fix regression caused by changes in Display for DataType [arrow-rs]
etseidl opened a new pull request, #8890: URL: https://github.com/apache/arrow-rs/pull/8890 # Which issue does this PR close? - Closes #8883. # Rationale for this change Second attempt at fixing #8883. # What changes are included in this PR? This changes `Display` for `DataType` to indicate non-nullable fields as "nonnull", and removes the "nullable" indicator for nullable fields. This is remain backwards compatible with previous behavior. # Are these changes tested? Should be handled by existing tests # Are there any user-facing changes? No, this changes un-released behavior -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
