You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/09/14 11:40:52 UTC
[GitHub] [arrow-datafusion] Ted-Jiang opened a new pull request, #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Ted-Jiang opened a new pull request, #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3476.
I think we can set true with only one col with pageIndex which is ordered. 🤔
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Ted-Jiang commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970694423
##########
datafusion/core/Cargo.toml:
##########
@@ -79,6 +79,7 @@ object_store = "0.5.0"
ordered-float = "3.0"
parking_lot = "0.12"
parquet = { version = "22.0.0", features = ["arrow", "async"] }
+parquet-format = "4.0.0"
Review Comment:
Should we move this logic to `arrow-rs` to avoid import this `parquet-format `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970807000
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
Why the filter for `fixed length byte array` is not efficient?
In the parquet, spark will write decimal(20,2) and encode the value as `fixed length byte array`, we can use the oder to accelerate the filter like `int32,int64`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat] Support `columns_sorted` in row_filters with pageIndex [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#issuecomment-1830642928
Closing as this PR is over a year old. Please feel free to reopen it / rebase it if you plan to keep working on it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat] Support `columns_sorted` in row_filters with pageIndex [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
URL: https://github.com/apache/arrow-datafusion/pull/3477
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972014565
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
Review Comment:
> Implies to me that `BoundaryOrder` is related to the ordering of the elements in `min_values` and `max_values` lists of the metadata and not the ordering of the actual data in the pages.
I think you got it. I have read the page index write in the java version which use the order of the `min_values` and `max_values` to determine the `boundary_order `.
But the comparator is core element to determine which is the MIN and which is the MAX.
For example, when we use the physical type ` fixed_length_byte_array ` to store the logical type `decimal(20,0)`, the `min_value` and `max_value` should be determined by the comparator for `decimal` not for the byte array.
```
let value = 1_i32;
let data = value.to_be_bytes();
println!("{:?}", data); // [0, 0, 0, 1]
let value = -1_i32;
let data = value.to_be_bytes();
println!("{:?}", data); [255, 255, 255, 255]
```
In the above code, `1_i32` is larger than `-1_i32`, but the form of bytes is the opposite
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970742874
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
Do we need to support `bool, bytearray, fixed length byte array` future？
we can file a new issue to track this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Ted-Jiang commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970694423
##########
datafusion/core/Cargo.toml:
##########
@@ -79,6 +79,7 @@ object_store = "0.5.0"
ordered-float = "3.0"
parking_lot = "0.12"
parquet = { version = "22.0.0", features = ["arrow", "async"] }
+parquet-format = "4.0.0"
Review Comment:
Should we move this logic to `arrow-rs` to avoid import this `parquet `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970742874
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
Do we need to support `bool, bytearray, fixed length byte array` in the future？
the physical of FIXED_LEN_BYTE_ARRAY type can be converted to many logical data type for parquet.
The order for the column is defined in https://github.com/apache/parquet-format/blob/54e53e5d7794d383529dd30746378f19a12afd58/src/main/thrift/parquet.thrift#L861
https://github.com/apache/parquet-format/blob/54e53e5d7794d383529dd30746378f19a12afd58/src/main/thrift/parquet.thrift#L219
https://github.com/apache/parquet-format/blob/54e53e5d7794d383529dd30746378f19a12afd58/src/main/thrift/parquet.thrift#L202
we can file a new issue to track this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Ted-Jiang commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r971462528
##########
datafusion/core/Cargo.toml:
##########
@@ -79,6 +79,7 @@ object_store = "0.5.0"
ordered-float = "3.0"
parking_lot = "0.12"
parquet = { version = "22.0.0", features = ["arrow", "async"] }
+parquet-format = "4.0.0"
Review Comment:
Sure!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] thinkharderdev commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
thinkharderdev commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970704591
##########
datafusion/core/Cargo.toml:
##########
@@ -79,6 +79,7 @@ object_store = "0.5.0"
ordered-float = "3.0"
parking_lot = "0.12"
parquet = { version = "22.0.0", features = ["arrow", "async"] }
+parquet-format = "4.0.0"
Review Comment:
I think that makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970813931
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
I am not the expert in the parquet column ColumnOrder, but from the document mentioned above, the order is not the nature order.
The order is determined by logical type not the physical type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972045090
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
I missing a most important point for that.
If we want to determine the column in the file is order, we should make use this column is order in each row group.
But this code just check the first group, it is not enough.
@Ted-Jiang @alamb @thinkharderdev
Please hold this pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972014565
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
Review Comment:
> Implies to me that `BoundaryOrder` is related to the ordering of the elements in `min_values` and `max_values` lists of the metadata and not the ordering of the actual data in the pages.
I think you got it. I have read the page index write in the java version which use the order of the `min_values` and `max_values` to determine the `boundary_order `. The `boundary_order` just indicate the order in the page level not in the data level (each element in the column).
For example, the below data in each page, `[1,3,8,4] [10,20,12] [30,40,33]` page data will generate the `ASCENDING` boundary_order, but data is unsorted.
We can get the benefit from boundary_order which can accelerate the filter page using the binary search, if the boundary_order is `ASCENDING` or `DESCENDING`
But the comparator is core element to determine which is the MIN and which is the MAX in one page.
For example, when we use the physical type ` fixed_length_byte_array ` to store the logical type `decimal(20,0)`, the `min_value` and `max_value` should be determined by the comparator for `decimal` not for the byte array.
```
let value = 1_i32;
let data = value.to_be_bytes();
println!("{:?}", data); // [0, 0, 0, 1]
let value = -1_i32;
let data = value.to_be_bytes();
println!("{:?}", data); [255, 255, 255, 255]
```
In the above code, `1_i32` is larger than `-1_i32`, but the form of bytes is the opposite
cc @alamb @Ted-Jiang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Ted-Jiang commented on pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#issuecomment-1246641502
@alamb @thinkharderdev @tustvold PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Ted-Jiang commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970766890
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
>Do we need to support bool, bytearray, fixed length byte array in the future？
I think not, in https://github.com/apache/arrow-datafusion/pull/3380, try put columns_sorted col ahead to filter row as much as possible, I think ` bool, bytearray, fixed length byte array` col using `page_index` 's min_max is not an `efficient` filter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r970807000
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
Why the filter for `fixed length byte array` is not efficient?
In the parquet, spark will write decimal(20,2) and encode the value as `fixed length byte array`, we can use the order to accelerate the filter like `int32,int64`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972045090
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
+ // For now we only set sorted with single col with pageIndex
+ if columns.len() == 1 && metadata.page_indexes().is_some() {
+ if let Some(indexes) = metadata.page_indexes().unwrap().get(0) {
+ return match indexes.get(columns[0]).unwrap() {
+ Index::NONE
+ | Index::BOOLEAN(_)
+ | Index::BYTE_ARRAY(_)
+ | Index::FIXED_LEN_BYTE_ARRAY(_) => Ok(false),
Review Comment:
I missing a most important point for that.
If we want to determine the column in the file is order, we should make sure this column is order in each row group.
But this code just check the first group, it is not enough.
@Ted-Jiang @alamb @thinkharderdev
Please hold this pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r971222798
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
Review Comment:
I may be misreading the spec, but the documentation on
https://github.com/apache/parquet-format/blob/54e53e5d7794d383529dd30746378f19a12afd58/src/main/thrift/parquet.thrift#L948-L959
```
/**
* Stores whether both min_values and max_values are orderd and if so, in
* which direction. This allows readers to perform binary searches in both
* lists. Readers cannot assume that max_values[i] <= min_values[i+1], even
* if the lists are ordered.
*/
4: required BoundaryOrder boundary_order
```
Implies to me that `BoundaryOrder` is related to the ordering of the elements in `min_values` and `max_values` lists of the metadata and not the ordering of the actual data in the pages. Give the comments on `min_values` and `max_values` it seems like those can have elements that are not in the actual file itself
So I wonder can one conclude that the data is sorted even if `BoundaryOrder` is specified? 🤔
##########
datafusion/core/Cargo.toml:
##########
@@ -79,6 +79,7 @@ object_store = "0.5.0"
ordered-float = "3.0"
parking_lot = "0.12"
parquet = { version = "22.0.0", features = ["arrow", "async"] }
+parquet-format = "4.0.0"
Review Comment:
I think parquet `23.0.0` will have properly exposed arrow definitions (added by @tustvold in https://github.com/apache/arrow-rs/pull/2626)
Though now that I see this, I see the proposal is probably to move the functions `columns_sorted` and `check_is_ordered` into the `parquet` crate which makes sense to me (and will likely get reviewed by some others with more parquet knowledge)
@Ted-Jiang can you file a ticket / PR to do so?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Ted-Jiang commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r971474454
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
Review Comment:
>So I wonder can one conclude that the data is sorted even if BoundaryOrder is specified? 🤔
Mostly we can assume is sorted, like in `parquet-testing` use `parquet-tools column-index ./alltypes_tiny_pages.parquet` we can find all col with `BoundaryOrder ` are sorted.😂
As we assume `columns_sorted ` as
```
Sorted columns may be queried more efficiently in the presence of a PageIndex.
```
I think use Page-index `BoundaryOrder ` is reasonable🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972014565
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
Review Comment:
> Implies to me that `BoundaryOrder` is related to the ordering of the elements in `min_values` and `max_values` lists of the metadata and not the ordering of the actual data in the pages.
I think you got it. I have read the page index write in the java version which use the order of the `min_values` and `max_values` to determine the `boundary_order `. The `boundary_order` just indicate the order in the page level not in the data level (each element in the column).
For example, the below data in each page, `[1,3,8,4] [10,20,12] [30,40,33]` page data will generate the `ASCENDING` boundary_order, but data is unsorted.
We can get the benefit from boundary_order which can accelerate the filter page using the binary search, if the boundary_order is `ASCENDING` or `DESCENDING`
But the comparator(ColumnOrder) is core element to determine which is the MIN and which is the MAX in one page.
For example, when we use the physical type ` fixed_length_byte_array ` to store the logical type `decimal(20,0)`, the `min_value` and `max_value` should be determined by the comparator for `decimal` not for the byte array.
```
let value = 1_i32;
let data = value.to_be_bytes();
println!("{:?}", data); // [0, 0, 0, 1]
let value = -1_i32;
let data = value.to_be_bytes();
println!("{:?}", data); [255, 255, 255, 255]
```
In the above code, `1_i32` is larger than `-1_i32`, but the form of bytes is the opposite
cc @alamb @Ted-Jiang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972037376
##########
datafusion/core/src/physical_plan/file_format/row_filter.rs:
##########
@@ -228,11 +230,32 @@ fn size_of_columns(columns: &[usize], metadata: &ParquetMetaData) -> Result<usiz
/// For a given set of `Column`s required for predicate `Expr` determine whether all
/// columns are sorted. Sorted columns may be queried more efficiently in the presence of
/// a PageIndex.
-fn columns_sorted(_columns: &[usize], _metadata: &ParquetMetaData) -> Result<bool> {
- // TODO How do we know this?
+fn columns_sorted(columns: &[usize], metadata: &ParquetMetaData) -> Result<bool> {
Review Comment:
Maybe i can try to find the java code, and it shows how to generate the `boundary_order` for one ColumnIndex.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #3477: [feat] Support `columns_sorted` in row_filters with pageIndex
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3477:
URL: https://github.com/apache/arrow-datafusion/pull/3477#issuecomment-1249662916
marking this PR as a draft pending resolution of @liukun4515 's review: https://github.com/apache/arrow-datafusion/pull/3477#discussion_r972045090
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org