Skip to content

Conversation

@alamb
Copy link

@alamb alamb commented Apr 29, 2021

Proposed modification for apache#236

}),
Some(3),
vec![None, None, Some(2)],
vec![None, Some(2), Some(0)],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the checked in answers are incorrect -- namely the first three items in the input are None, 0 and 2 and yet the output has two nulls in it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this passing? This looks wrong to me, the unoptimized order (ie /wo limit) would look like None, None, 2, 0, 0, -1, applying the limit: None, None, 2.

if let Some(limit) = limit {
len = limit.min(len);
// count how many nulls are present in the limit
nulls_len = nulls.iter().filter(|&idx| *idx as usize <= limit).count();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else in this function doesn't need to dig into the actual index values so something smells here to me. I thought the limit is applied to the sorted result so I don't know that comparing the limit to the indices is correct here.

}),
Some(3),
vec![None, None, Some(2)],
vec![None, Some(2), Some(0)],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this passing? This looks wrong to me, the unoptimized order (ie /wo limit) would look like None, None, 2, 0, 0, -1, applying the limit: None, None, 2.

@alamb
Copy link
Author

alamb commented Apr 29, 2021

I misunderstood the expected behavior. Sorry about that

@alamb alamb closed this Apr 29, 2021
medwards pushed a commit that referenced this pull request Jun 7, 2021
* Add ported Rust release verification script

* Minor simplifications. (#1)

Co-authored-by: Jorge Leitao <[email protected]>
@alamb alamb deleted the alamb/fix_for_fix branch June 15, 2021 11:16
medwards pushed a commit that referenced this pull request Sep 5, 2022
* add parquet-fromcsv (#1)

add command line tool for convert csv to parquet.

* add `text` for non-rust documentation text

* Update parquet/src/bin/parquet-fromcsv.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

* Update parquet/src/bin/parquet-fromcsv.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

* Update parquet/src/bin/parquet-fromcsv.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

* Update parquet/src/bin/parquet-fromcsv.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

* automate update help text

* remove anyhow

* add rat_exclude_files

* update test_command_help

* fix clippy warnings

* add writer-version, max-row-group-size arg

* fix cargo fmt lint

Co-authored-by: Raphael Taylor-Davies <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants