Remove INT96 timestamps in cuDF Parquet writer #15901
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
libcudf
mhaseeb123
added
2 - In Progress
Merged
9 tasks
etseidl
reviewed
May 31, 2024
|
Thanks for this (IMO long overdue) update! Looks good. 👍 |
…eb123/cudf into deprecate-int96-timestamps
|
/ok to test |
mhaseeb123
changed the title
|
/ok to test |
|
Hi @nvdbaranec, I wanted to bring your attention to the changes in this PR (removes INT96 timestamps in cuDF) and wanted to ask if it would create any waves across Spark functionality or testing? |
mhaseeb123
added
5 - DO NOT MERGE
|
Closing the PR as we need INT96 timestamps for Spark. |
rapids-bot bot
pushed a commit
that referenced
this pull request
Jul 9, 2024
…ation` types with Arrow (#15875) Closes #15847 This PR adds the support to construct and write base64-encoded serialized `arrow:schema`-type IPC message to parquet file footer to allow faithfully roundtrip with Arrow via Parquet for `duration` type. ### Answered - [x] Only construct and write `arrow:schema` if asked by the user via `store_schema` argument (cudf) or `write_arrow_schema` (libcudf). i.e. Default these variables to `false` otherwise. - [x] The internal/libcudf variable name for `store_schema` can stay `write_arrow_schema` and it should be fine. This has been done to disambiguate which schema (arrow or parquet) we are talking about. - [x] Separate PR: `int96_timestamps` cannot be deprecated/removed in cuDF as Spark is actively using it. #15901 - [x] cuDF Parquet writer supports `decimal32` and `decimal64` [fixed types](https://github.com/rapidsai/cudf/blob/branch-24.08/cpp/src/io/parquet/writer_impl.cu#L561). These are not directly supported by Arrow so we will [convert](https://github.com/rapidsai/cudf/blob/branch-24.08/cpp/src/interop/to_arrow.cu#L155) `decimal32/decimal64` columns to `decimal128`. - [x] `is_col_nullable()` function moved to `writer_impl_helpers.cpp` along with some other helper functions. - [x] A common `convert_data_to_decimal128` can be separated out and used in `writer_impl.cu` and `to_arrow.cu`. Tracking in a separate issue. #16194 CC @vuule @etseidl @nvdbaranec @GregoryKimball @galipremsagar for vis. Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Thomas Li (https://github.com/lithomas1) - GALI PREM SAGAR (https://github.com/galipremsagar) - Vukasin Milovanovic (https://github.com/vuule) - Bradley Dice (https://github.com/bdice) URL: #15875
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Partially closes #15847.
Closes #10438
This PR remonves INT96 timestamps from cuDF in accordance with Arrow. For backward compatibility and robustness, the Parquet reader still has the capability to read INT96 timestamps and convert to INT64. For more discussion, see #15875.
Discussion
Answered
CC @vuule @etseidl @nvdbaranec @GregoryKimball @galipremsagar for vis.
Checklist