fix UNION ALL bug: thread 'main' panicked at 'index out of bounds: the len is 1 but the index is 1', ./src/datatypes/schema.rs:165:10 #1088
Conversation
|
Finally, I found the real bug is at optimizing phrase where PTAL,thanks @alamb @Dandandan @houqp |
| let mut new_schema = Vec::new(); | ||
| for df_field in schema.fields() { | ||
| if union_required_fields.contains(df_field.field()) { | ||
| new_schema.push(df_field.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't know if you are into the whole functional programming thing, but you can probably write this without a mut Vec like
let new_schema = DFSchema::new(
schema.fields()
.iter()
.filter(|df_field| {
union_required_fields.contains(df_field.field())
})
.cloned()
.collect()
)?;I don't think it makes any practical difference (what you have here is fine too) I just figured I would point it out
There was a problem hiding this comment.
I would prefer us to go functional ere to keep the style consistent within the code base :)
There was a problem hiding this comment.
Yes, I also like the cool and concise functional style. But because I'm not familiar with functional programming style, I chose conservative writing at the beginning. Thanks for your suggestion!
There was a problem hiding this comment.
Yeah it took me a while to become comfortable with the functional style.
As an aside - I also later realized that using this functional style is more than just about code structure / readability -- it is one of the key ways Rust does fast and safe: in the mut Vec<..> version, every time data is pushed the capacity of the vec needs to be checked and extended if not large enough. In the iter() version often the Vec can be created up front and then written directly into.
| let mut new_schema = Vec::new(); | ||
| for df_field in schema.fields() { | ||
| if union_required_fields.contains(df_field.field()) { | ||
| new_schema.push(df_field.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I would prefer us to go functional ere to keep the style consistent within the code base :)
| async fn union_all_with_aggregate() -> Result<()> { | ||
| let mut ctx = ExecutionContext::new(); | ||
| let sql = | ||
| "SELECT SUM(d) FROM (SELECT 1 as c, 2 as d UNION ALL SELECT 1 as c, 3 AS d) as a"; |
…e len is 1 but the index is 1', ./src/datatypes/schema.rs:165:10
xudong963
force-pushed
the
debug_union_all
branch
from
e2f2690 to
66dc838
Compare
|
Bug fix merged, thanks @xudong963! |
|
Thanks again @xudong963 ! |
Which issue does this PR close?
Closes #1064
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?