You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jorisvandenbossche (via GitHub)" <gi...@apache.org> on 2023/12/04 16:40:01 UTC

[PR] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

jorisvandenbossche opened a new pull request, #39065:
URL: https://github.com/apache/arrow/pull/39065

   ### Rationale for this change
   
   Currently when filtering with a nested field reference, we were taking the corresponding parquet SchemaField for just the first index of the nested path, i.e. the parent node in the Parquet schema. But logically, filtering on statistics only works for a primitive leaf node.
   
   This PR changes that logic to iterate over all indices of the FieldPath, if nested, to ensure we use the actual corresponding child leaf node of the ParquetSchema to get the statistics from.
   
   ### Are there any user-facing changes?
   
   No, only improving performance by doing the filtering at the row group stage, instead of afterwards on the read data


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1416949294


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;

Review Comment:
   Because initially I wasn't using a pointer, and that didn't work when declared inside the loop (it kept the value of the previous iteration), and then moved it out to try to fix, but the solution was to use a pointer instead, but that indeed works fine with keeping it in the loop ;) Will update.



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1444797018


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -897,16 +907,25 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
 
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    const SchemaField* schema_field = &manifest_->schema_fields[match[0]];

Review Comment:
   I would like to merge this before the 15.0 branch cut-off, so going to merge as is, but will look into factoring it out as a helper function in a follow-up!



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39065:
URL: https://github.com/apache/arrow/pull/39065#issuecomment-1882192540

   After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit ffcfabdb956d72707557a1fcf113c6b7cb118f50.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/20285345547) has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1414195732


##########
cpp/src/arrow/dataset/file_parquet.h:
##########
@@ -175,7 +175,7 @@ class ARROW_DS_EXPORT ParquetFileFragment : public FileFragment {
   Result<std::shared_ptr<Fragment>> Subset(std::vector<int> row_group_ids);
 
   static std::optional<compute::Expression> EvaluateStatisticsAsExpression(
-      const Field& field, const parquet::Statistics& statistics);
+      const Field& field, const FieldRef& field_ref, const parquet::Statistics& statistics);

Review Comment:
   This is public API? So I can add a variant with the original signature that creates a FieldRef from the Field? 



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1432485419


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -897,16 +907,25 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
 
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    const SchemaField* schema_field = &manifest_->schema_fields[match[0]];

Review Comment:
   You mean including the for loop below, right? 
   
   I think a method on the SchemaManifest might be a logical place to have this. It already has a `GetColumnField` to return a SchemaField based on a single integer index. There could be a variant which accepts a FieldPath



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1416924067


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;
   for (const FieldRef& ref : FieldsInExpression(predicate)) {
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
-
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    schema_field = &manifest_->schema_fields[match[0]];
+
+    for (size_t i = 1; i < match.indices().size(); ++i) {
+      if (schema_field->field->type()->id() != Type::STRUCT) {
+        return Status::Invalid("nested paths only supported for structs");
+      }

Review Comment:
   Yes, but we currently also don't support any predicate kernels for those data types at the moment AFAIK. 
   
   For example for a list column, you can't do something like "list_field > 1" because 1) such kernel isn't implemented, and 2) that actually also doesn't really make sense as a list scalar contains multiple values, so that doesn't evaluate to simple True/False, you need some kind of aggregation like "elementwise_all(list_field > 1)" (i.e. are "all" (or any) values in a list scalar larger than 1). 
   And even then simplifying such more complex expression based on the parquet statistics would also need to be implemented.
   
   (I would like to see this work at some point, but that's certainly future work)



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #39065:
URL: https://github.com/apache/arrow/pull/39065#issuecomment-1842269106

   I'll take a quick look now


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1416932620


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;
   for (const FieldRef& ref : FieldsInExpression(predicate)) {
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
-
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    schema_field = &manifest_->schema_fields[match[0]];
+
+    for (size_t i = 1; i < match.indices().size(); ++i) {
+      if (schema_field->field->type()->id() != Type::STRUCT) {
+        return Status::Invalid("nested paths only supported for structs");
+      }

Review Comment:
   I agree list/map filter is so hard to filtering, which might need extra predicates. Let disable it now, but maybe we can test some more complex struct?



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1414193654


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -357,8 +357,8 @@ Result<bool> IsSupportedParquetFile(const ParquetFileFormat& format,
 }  // namespace
 
 std::optional<compute::Expression> ParquetFileFragment::EvaluateStatisticsAsExpression(
-    const Field& field, const parquet::Statistics& statistics) {
-  auto field_expr = compute::field_ref(field.name());
+    const Field& field, const FieldRef& field_ref, const parquet::Statistics& statistics) {
+  auto field_expr = compute::field_ref(field_ref);

Review Comment:
   I don't know if there is a better way, but the reason I am passing the FieldRef through the various function calls (through ColumnChunkStatisticsAsExpression to EvaluateStatisticsAsExpression), is that once we are here and we have the "schild" field, we don't know what the original full nested field was. 
   
   Neither the SchemaField passed to `ColumnChunkStatisticsAsExpression` nor the Field (`schema_field.field`) passed to `EvaluateStatisticsAsExpression` currently have that information AFAIK. 



##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -357,8 +357,8 @@ Result<bool> IsSupportedParquetFile(const ParquetFileFormat& format,
 }  // namespace
 
 std::optional<compute::Expression> ParquetFileFragment::EvaluateStatisticsAsExpression(
-    const Field& field, const parquet::Statistics& statistics) {
-  auto field_expr = compute::field_ref(field.name());
+    const Field& field, const FieldRef& field_ref, const parquet::Statistics& statistics) {
+  auto field_expr = compute::field_ref(field_ref);

Review Comment:
   I don't know if there is a better way, but the reason I am passing the FieldRef through the various function calls (through ColumnChunkStatisticsAsExpression to EvaluateStatisticsAsExpression), is that once we are here and we have the "schild" field, we don't know what the original full nested field was. 
   
   Neither the SchemaField passed to `ColumnChunkStatisticsAsExpression` nor the Field (`schema_field.field`) passed to `EvaluateStatisticsAsExpression` currently have that information AFAIU. 



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1416924508


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;
   for (const FieldRef& ref : FieldsInExpression(predicate)) {
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
-
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    schema_field = &manifest_->schema_fields[match[0]];
+
+    for (size_t i = 1; i < match.indices().size(); ++i) {
+      if (schema_field->field->type()->id() != Type::STRUCT) {
+        return Status::Invalid("nested paths only supported for structs");
+      }

Review Comment:
   Would you mind add a test for the "List/Map" filter doesn't work in cpp



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1416873982


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;
   for (const FieldRef& ref : FieldsInExpression(predicate)) {
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
-
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    schema_field = &manifest_->schema_fields[match[0]];
+
+    for (size_t i = 1; i < match.indices().size(); ++i) {
+      if (schema_field->field->type()->id() != Type::STRUCT) {
+        return Status::Invalid("nested paths only supported for structs");
+      }

Review Comment:
   So this limit user passing an filter on Map/List?



##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -415,6 +418,12 @@ std::optional<compute::Expression> ParquetFileFragment::EvaluateStatisticsAsExpr
   return std::nullopt;
 }
 
+std::optional<compute::Expression> ParquetFileFragment::EvaluateStatisticsAsExpression(
+    const Field& field, const parquet::Statistics& statistics) {
+  const auto field_name = field.name();
+  return EvaluateStatisticsAsExpression(field, FieldRef(field_name), statistics);

Review Comment:
   style:
   ```suggestion
     return EvaluateStatisticsAsExpression(field, FieldRef(std::move(field_name)), statistics);
   ```



##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;

Review Comment:
   Why move the declaration of `schema_field` outside the loop?



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1417269332


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -893,20 +902,29 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     return std::vector<compute::Expression>{};
   }
 
+  const SchemaField* schema_field = nullptr;
   for (const FieldRef& ref : FieldsInExpression(predicate)) {
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
-
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    schema_field = &manifest_->schema_fields[match[0]];
+
+    for (size_t i = 1; i < match.indices().size(); ++i) {
+      if (schema_field->field->type()->id() != Type::STRUCT) {
+        return Status::Invalid("nested paths only supported for structs");
+      }

Review Comment:
   > Would you mind add a test for the "List/Map" filter doesn't work in cpp
   
   Filtering with a list or map field actually already fails in an earlier step, when binding the filter expression to the schema (and binding isn't done in `FilterRowGroups`, it's expected to already be done, also in the test for this it is done up front in the test setup code).



-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche merged PR #39065:
URL: https://github.com/apache/arrow/pull/39065


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39065:
URL: https://github.com/apache/arrow/pull/39065#issuecomment-1883421230

   Revision: fb10913cf3c6b9d7e681d7647abe7ecfd7fdd5b0
   
   Submitted crossbow builds: [ursacomputing/crossbow @ actions-e8de21cfb7](https://github.com/ursacomputing/crossbow/branches/all?query=actions-e8de21cfb7)
   
   |Task|Status|
   |----|------|
   |wheel-macos-big-sur-cp310-arm64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-big-sur-cp310-arm64)](https://github.com/ursacomputing/crossbow/actions/runs/7464237059/job/20310763894)|
   |wheel-macos-big-sur-cp311-arm64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-big-sur-cp311-arm64)](https://github.com/ursacomputing/crossbow/actions/runs/7464237167/job/20310764335)|
   |wheel-macos-big-sur-cp312-arm64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-big-sur-cp312-arm64)](https://github.com/ursacomputing/crossbow/actions/runs/7464236926/job/20310763218)|
   |wheel-macos-big-sur-cp38-arm64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-big-sur-cp38-arm64)](https://github.com/ursacomputing/crossbow/actions/runs/7464236922/job/20310763290)|
   |wheel-macos-big-sur-cp39-arm64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-big-sur-cp39-arm64)](https://github.com/ursacomputing/crossbow/actions/runs/7464236879/job/20310763223)|
   |wheel-macos-catalina-cp310-amd64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-catalina-cp310-amd64)](https://github.com/ursacomputing/crossbow/actions/runs/7464236992/job/20310763631)|
   |wheel-macos-catalina-cp311-amd64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-catalina-cp311-amd64)](https://github.com/ursacomputing/crossbow/actions/runs/7464236870/job/20310763115)|
   |wheel-macos-catalina-cp312-amd64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-catalina-cp312-amd64)](https://github.com/ursacomputing/crossbow/actions/runs/7464237168/job/20310764265)|
   |wheel-macos-catalina-cp38-amd64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-catalina-cp38-amd64)](https://github.com/ursacomputing/crossbow/actions/runs/7464237092/job/20310763991)|
   |wheel-macos-catalina-cp39-amd64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-e8de21cfb7-github-wheel-macos-catalina-cp39-amd64)](https://github.com/ursacomputing/crossbow/actions/runs/7464237120/job/20310763942)|


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #39065:
URL: https://github.com/apache/arrow/pull/39065#issuecomment-1883416460

   @github-actions crossbow submit wheel-macos-*


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39065:
URL: https://github.com/apache/arrow/pull/39065#discussion_r1431549725


##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -897,16 +907,25 @@ Result<std::vector<compute::Expression>> ParquetFileFragment::TestRowGroups(
     ARROW_ASSIGN_OR_RAISE(auto match, ref.FindOneOrNone(*physical_schema_));
 
     if (match.empty()) continue;
-    if (statistics_expressions_complete_[match[0]]) continue;
-    statistics_expressions_complete_[match[0]] = true;
+    const SchemaField* schema_field = &manifest_->schema_fields[match[0]];

Review Comment:
   Since this is the same logic as [FieldPath::Get](https://github.com/apache/arrow/blob/42058376eed67019e6bea5715ae3740711266fd1/cpp/src/arrow/type.cc#L1785), would you mind extracting it as a separate function? It would be nice to have a clear single entry point for future work on nested field references in 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


Re: [PR] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39065:
URL: https://github.com/apache/arrow/pull/39065#issuecomment-1839035514

   :warning: GitHub issue #39064 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-39064: [C++][Parquet] Support row group filtering for nested paths for struct fields [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39065:
URL: https://github.com/apache/arrow/pull/39065#issuecomment-1839035537

   :warning: GitHub issue #39064 **has no components**, please add labels for components.


-- 
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