Fix regression caused by changes in Display for DataType - display (List(non-null Int64) instead of List(nullable Int64)#8890
Conversation
|
@alamb here's my second attempt. Who else should take a look at this one? |
arrow-schema/src/datatype_parse.rs
Outdated
| let tok = self | ||
| .tokenizer | ||
| .next_if(|next| matches!(next, Ok(Token::NonNull | Token::Nullable))); | ||
| match tok { | ||
| Some(Ok(Token::NonNull)) => false, | ||
| _ => true, | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
I don't think we have released anything with nullable in it
|
will check this out in a few moments |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
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. |
I have played around with it and did some more research, and I would like to propose yet another thing: |
|
Here is a proposed change: |
|
I pushed a small commit with a regression test |
This reverts commit 6c26f1b.
|
Oh no -- I liked the |
This reverts commit e4ce1dc.
Rename 'nonnull' to 'non-null'
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 😄 |
|
thank you ... testing now. |
alamb
changed the title
List(non-null Int64) instead of List(nullable Int64)
|
I'll merge once CI finishes |
|
|
|
🎉 |
2 participants
Which issue does this PR close?
List(Int64)results in nullable list in 57.0.0 and a non-nullable list in 57.1.0 #8883.Rationale for this change
Second attempt at fixing #8883.
What changes are included in this PR?
This changes
DisplayforDataTypeto indicate non-nullable fields as "nonnull", and removes the "nullable" indicator for nullable fields. This is to 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