Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix duration conversion error #5626

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Liyixin95
Copy link
Contributor

Which issue does this PR close?

Closes #5625

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Apr 11, 2024
@tustvold
Copy link
Contributor

Could we get a test of this please

@alamb
Copy link
Contributor

alamb commented Apr 12, 2024

I agree -- a test is important to make sure we don't break this fix in some future refactor

@Liyixin95
Copy link
Contributor Author

Could we get a test of this please

sorry for the noob question, where should I add a test for it?

@alamb
Copy link
Contributor

alamb commented Apr 13, 2024

Could we get a test of this please

sorry for the noob question, where should I add a test for it?

I think we could add a round trip test here perhaps:

#[test]
fn date32_single_column() {
required_and_optional::<Date32Array, _>(0..SMALL_SIZE as i32);
}
#[test]
fn date64_single_column() {
// Date64 must be a multiple of 86400000, see ARROW-10925
required_and_optional::<Date64Array, _>(
(0..(SMALL_SIZE as i64 * 86400000)).step_by(86400000),
);
}

@Liyixin95
Copy link
Contributor Author

Could we get a test of this please

sorry for the noob question, where should I add a test for it?

I think we could add a round trip test here perhaps:

#[test]
fn date32_single_column() {
required_and_optional::<Date32Array, _>(0..SMALL_SIZE as i32);
}
#[test]
fn date64_single_column() {
// Date64 must be a multiple of 86400000, see ARROW-10925
required_and_optional::<Date64Array, _>(
(0..(SMALL_SIZE as i64 * 86400000)).step_by(86400000),
);
}

write duration to parquet get a panic...

DataType::Duration(_) => Err(arrow_err!("Converting Duration to parquet not supported",)),

@tustvold
Copy link
Contributor

tustvold commented Apr 15, 2024

write duration to parquet get a panic...

Yes I think we would need #1938. Technically polars is doing something a little funky here

@alamb
Copy link
Contributor

alamb commented Apr 16, 2024

or put another way there is no way to write a duration to a parquet file with arrow-rs yet (but polars can do so).

So the ideal solution is support writing duration to parquet too (but I suspect there might be work to sort out what the desired behavior is)

Perhaps for this PR we can just check in a small parquet file that was written by polars and ensure we can read it back correctly 🤔

@tustvold tustvold marked this pull request as draft May 8, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

write duration to parquet but read as int64
3 participants