Conversation
github-actions
bot
added
core
| ]; | ||
| assert_batches_eq!(expected, &result); | ||
| let err = pretty_format_batches(&result).err().unwrap().to_string(); | ||
| assert_eq!(err, "Parser error: Invalid timezone \"+08:00:00\": Expected format [+-]XX:XX, [+-]XX, or [+-]XXXX"); |
There was a problem hiding this comment.
Invalid timezones now result in an error when creating the formatter, instead of rendering the invalid timezone to each string. I'm not sure how people feel about this, I think it is better to fail loud but welcome thoughts
There was a problem hiding this comment.
I think it is ok -- cc @comphead and @waitingkuo
There was a problem hiding this comment.
sorry for the late reply. this looks great to me. I originally followed how pyarrow works to implement it. raising parser error makes more sense to me.
| ]; | ||
| assert_batches_eq!(expected, &result); | ||
| let err = pretty_format_batches(&result).err().unwrap().to_string(); | ||
| assert_eq!(err, "Parser error: Invalid timezone \"+08:00:00\": Expected format [+-]XX:XX, [+-]XX, or [+-]XXXX"); |
There was a problem hiding this comment.
I think it is ok -- cc @comphead and @waitingkuo
Eep, will investigate, shenanigans may be afoot. Might be a double-panic |
|
Shenanigans did abound - apache/arrow-rs#3691, fix in apache/arrow-rs#3692 |
Testing for the win! |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I am ok with these changes, but I worry about the possible effects on our downstream users (e.g. @maxburke was just discussing the upgrade pain earlier today on ASF slack).
What would you think about sending a note to the mailing lists / slack channels giving a heads up and asking for feedback? I can do the communicating if you like
| "+---+-----+-----------------+", | ||
| "| a | b | COUNT(1)[count] |", | ||
| "+---+-----+-----------------+", | ||
| "| | 1.0 | 2 |", |
There was a problem hiding this comment.
these changes are going to create some serious downstream churn I suspect. I wonder if we should wait until apache/arrow-rs#3717 is released (so downstream crates can choose to have the old behavior) 🤔
There was a problem hiding this comment.
@alamb is the issue related to column name [count]?
There was a problem hiding this comment.
Coming back to this, even with apache/arrow-rs#3717 floats will be formatted with a decimal point, there isn't an option currently exposed to configure this...
Yes please, my 2 cents is optimising for stability is premature at this point, but welcome other input |
| "+--------------------+----------+", | ||
| "| 0.9294097332465232 | 1 |", | ||
| "| 0.3114712539863804 | 1 |", | ||
| "| 0.9294097332465232 | 1.0 |", |
There was a problem hiding this comment.
I am confused about this changes？
cc @alamb @tustvold
As you said in https://github.com/apache/arrow-datafusion/pull/5241/files#r1106342898.
There was a problem hiding this comment.
What are you confused about? Despite its name, this column is a floating point type and so is formatted with a decimal point?
There was a problem hiding this comment.
Despite its name
I certainly found that confusing 😆
There was a problem hiding this comment.
What are you confused about? Despite its name, this column is a floating point type and so is formatted with a decimal point?
I missed the data type from the name of this column
|
I posted a note to the mailing list: https://lists.apache.org/thread/9y6bhdj2vgo9ll7bj72mf5gpwxkqy2b1 |
|
Unless there are any comments, I think we should merge this next week |
|
Benchmark runs are scheduled for baseline = c9d4eac and contender = e3679e2. e3679e2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* Update arrow 33 * Fix test * Fix avro * Update pin * Format * Further fixes * Remove pin
6 participants
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?