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 2021/02/19 19:52:32 UTC
[GitHub] [arrow] bkietz opened a new pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection [WIP]
bkietz opened a new pull request #9532:
URL: https://github.com/apache/arrow/pull/9532
[x] Be more explicit about scan time schemas: dataset (aka reader) and projected schemas are independent
[x] Ensure field metadata and other attributes are preserved by projection exprs
[x] Replace RecordBatchProjector with a projection exprs
[ ] Rewrite ScannerBuilder::Project to accept arbitrary projections (not just subset-of-fields)
[ ] Ensure that stacked projection exprs are simplifiable to a single project expr
[ ] Refactor UnionDataset to support reconciling its children with projections
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r583924317
##########
File path: cpp/src/arrow/dataset/expression_test.cc
##########
@@ -1135,5 +1175,36 @@ TEST(Expression, SerializationRoundTrips) {
equal(field_ref("beta"), literal(3.25f))}));
}
+TEST(Projection, AugmentWithNull) {
+ auto just_i32 = ArrayFromJSON(struct_({kBoringSchema->GetFieldByName("i32")}),
+ R"([{"i32": 0}, {"i32": 1}, {"i32": 2}])");
+
+ {
+ ASSERT_OK_AND_ASSIGN(auto proj, project({field_ref("f64"), field_ref("i32")},
+ {"projected double", "projected int"})
+ .Bind(*kBoringSchema));
+
+ auto expected = ArrayFromJSON(
+ struct_({field("projected double", float64()), field("projected int", int32())}),
+ R"([[null, 0], [null, 1], [null, 2]])");
+ ASSERT_OK_AND_ASSIGN(auto actual, ExecuteScalarExpression(proj, just_i32));
+
+ AssertDatumsEqual(Datum(expected), actual);
+ }
+
+ {
+ ASSERT_OK_AND_ASSIGN(
+ auto proj,
+ project({field_ref("f64")}, {"projected double"}).Bind(*kBoringSchema));
+
+ // NB: only a scalar was projected, this is *not* automatically broadcast to an array.
+ ASSERT_OK_AND_ASSIGN(auto expected, StructScalar::Make({MakeNullScalar(float64())},
Review comment:
Ah, I see your concern. Individual calls to project do not broadcast scalars in case subsequent steps in the pipeline want to do something more efficient. FilterAndProjectScanTask broadcasts scalars to the correct length before yielding the batch: https://github.com/apache/arrow/pull/9532/files?file-filters%5B%5D=.cc&file-filters%5B%5D=.h&file-filters%5B%5D=.java&file-filters%5B%5D=.pxd&file-filters%5B%5D=.py#diff-25b1bd283e8242f8384b24a0f1e8b61fbca0c2784ab679f9a2a00b03450487aaR72-R76
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582064969
##########
File path: cpp/src/arrow/dataset/expression_test.cc
##########
@@ -1135,5 +1175,36 @@ TEST(Expression, SerializationRoundTrips) {
equal(field_ref("beta"), literal(3.25f))}));
}
+TEST(Projection, AugmentWithNull) {
+ auto just_i32 = ArrayFromJSON(struct_({kBoringSchema->GetFieldByName("i32")}),
+ R"([{"i32": 0}, {"i32": 1}, {"i32": 2}])");
+
+ {
+ ASSERT_OK_AND_ASSIGN(auto proj, project({field_ref("f64"), field_ref("i32")},
+ {"projected double", "projected int"})
+ .Bind(*kBoringSchema));
+
+ auto expected = ArrayFromJSON(
+ struct_({field("projected double", float64()), field("projected int", int32())}),
+ R"([[null, 0], [null, 1], [null, 2]])");
+ ASSERT_OK_AND_ASSIGN(auto actual, ExecuteScalarExpression(proj, just_i32));
+
+ AssertDatumsEqual(Datum(expected), actual);
+ }
+
+ {
+ ASSERT_OK_AND_ASSIGN(
+ auto proj,
+ project({field_ref("f64")}, {"projected double"}).Bind(*kBoringSchema));
+
+ // NB: only a scalar was projected, this is *not* automatically broadcast to an array.
+ ASSERT_OK_AND_ASSIGN(auto expected, StructScalar::Make({MakeNullScalar(float64())},
Review comment:
I'll add a test for this case here as well
##########
File path: cpp/src/arrow/compute/kernels/scalar_nested.cc
##########
@@ -63,9 +63,15 @@ const FunctionDoc list_value_length_doc{
Result<ValueDescr> ProjectResolve(KernelContext* ctx,
const std::vector<ValueDescr>& descrs) {
const auto& names = OptionsWrapper<ProjectOptions>::Get(ctx).field_names;
- if (names.size() != descrs.size()) {
- return Status::Invalid("project() was passed ", names.size(), " field ", "names but ",
- descrs.size(), " arguments");
+ const auto& nullable = OptionsWrapper<ProjectOptions>::Get(ctx).field_nullability;
+ const auto& metadata = OptionsWrapper<ProjectOptions>::Get(ctx).field_metadata;
+
+ if (names.size() != descrs.size() || nullable.size() != descrs.size() ||
+ metadata.size() != descrs.size()) {
+ return Status::Invalid("project() was passed ", descrs.size(), " arguments but ",
+ names.size(), " field names, ", nullable.size(),
+ " nullability bits, and ", metadata.size(),
+ " nullability bits.");
Review comment:
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r583924716
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
Correct, `FieldsInExpression(projection)` will yield only materialized fields. That SQL's not supported; only fields in the dataset schema may be referenced
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#issuecomment-785456657
I've started testing in R with a [branch](https://github.com/apache/arrow/compare/master...nealrichardson:r-dataset-projection) (added commit [here](https://github.com/apache/arrow/commit/e77dc91920a7951c672f04dd9d2bb27e2e754424)) on top of this and my `mutate()` branch, and I've got a simple test working, but I'm running into a problem trying to apply this to our canonical taxi dataset query from the docs. If I add a column that is derived from other columns, it only yields valid data if I also include those columns in my result. So this works:
```
> ds %>%
+ filter(total_amount > 100, year == 2015) %>%
+ mutate(
+ tip_pct = 100 * tip_amount / total_amount
+ ) %>%
+ select(tip_pct, tip_amount, total_amount, passenger_count) %>%
+ group_by(passenger_count) %>%
+ collect() %>% head()
# A tibble: 6 x 4
# Groups: passenger_count [2]
tip_pct tip_amount total_amount passenger_count
<dbl> <dbl> <dbl> <int>
1 0 0 149. 1
2 16.6 22 132. 1
3 0 0 205. 1
4 7.27 10 138. 1
5 23.0 24.5 107. 1
6 11.2 12 107. 2
```
But what I want to do is this, so that I don't have to keep around the columns that I've already used to create my derived one:
```
> ds %>%
+ filter(total_amount > 100, year == 2015) %>%
+ mutate(
+ tip_pct = 100 * tip_amount / total_amount
+ ) %>%
+ select(tip_pct, passenger_count) %>%
+ group_by(passenger_count) %>%
+ collect() %>% head()
# A tibble: 6 x 2
# Groups: passenger_count [2]
tip_pct passenger_count
<dbl> <int>
1 NA 1
2 NA 1
3 NA 1
4 NA 1
5 NA 1
6 NA 2
```
To be clear, this latter example sends to `ScannerBuilder::Project` a vector of 2 Expressions, one for `tip_pct` that is derived from other fields in the data (but not included in the projection) and one for `passenger_count` that is just a `FieldRef`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r581365819
##########
File path: cpp/src/arrow/compute/kernels/scalar_nested.cc
##########
@@ -63,9 +63,15 @@ const FunctionDoc list_value_length_doc{
Result<ValueDescr> ProjectResolve(KernelContext* ctx,
const std::vector<ValueDescr>& descrs) {
const auto& names = OptionsWrapper<ProjectOptions>::Get(ctx).field_names;
- if (names.size() != descrs.size()) {
- return Status::Invalid("project() was passed ", names.size(), " field ", "names but ",
- descrs.size(), " arguments");
+ const auto& nullable = OptionsWrapper<ProjectOptions>::Get(ctx).field_nullability;
+ const auto& metadata = OptionsWrapper<ProjectOptions>::Get(ctx).field_metadata;
+
+ if (names.size() != descrs.size() || nullable.size() != descrs.size() ||
+ metadata.size() != descrs.size()) {
+ return Status::Invalid("project() was passed ", descrs.size(), " arguments but ",
+ names.size(), " field names, ", nullable.size(),
+ " nullability bits, and ", metadata.size(),
+ " nullability bits.");
Review comment:
`" nullability bits."` -> `" metadata."
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -130,12 +139,16 @@ TEST_F(TestCsvFileFormat, IsSupported) {
ASSERT_OK_AND_ASSIGN(supported, format_->IsSupported(*source));
ASSERT_EQ(supported, false);
- source = GetFileSource();
+ source = GetFileSource(R"(f64
+1.0
+
+N/A
+2)");
ASSERT_OK_AND_ASSIGN(supported, format_->IsSupported(*source));
EXPECT_EQ(supported, true);
}
-TEST_F(TestCsvFileFormat, DISABLED_NonMaterializedFieldWithDifferingTypeFromInferred) {
+TEST_F(TestCsvFileFormat, NonMaterializedFieldWithDifferingTypeFromInferred) {
Review comment:
What does NonMaterialized mean in this context? Maybe `ProjectedFieldDifferingTypeFromInferred`?
##########
File path: cpp/src/arrow/dataset/file_ipc_test.cc
##########
@@ -278,38 +289,32 @@ TEST_F(TestIpcFileFormat, ScanRecordBatchReaderProjected) {
}
TEST_F(TestIpcFileFormat, ScanRecordBatchReaderProjectedMissingCols) {
- auto reader_without_i32 = GetRecordBatchReader(
- schema({field("f64", float64()), field("i64", int64()), field("f32", float32())}));
-
- auto reader_without_f64 = GetRecordBatchReader(
- schema({field("i64", int64()), field("f32", float32()), field("i32", int32())}));
-
- auto reader =
- GetRecordBatchReader(schema({field("f64", float64()), field("i64", int64()),
- field("f32", float32()), field("i32", int32())}));
-
- schema_ = reader->schema();
- opts_ = ScanOptions::Make(schema_);
- opts_->projector = RecordBatchProjector(SchemaFromColumnNames(schema_, {"f64"}));
+ SetSchema({f64, i64, f32, i32});
+ ASSERT_OK(SetProjection(opts_.get(), {"f64"}));
opts_->filter = equal(field_ref("i32"), literal(0));
+ auto reader_without_i32 = GetRecordBatchReader(schema({f64, i64, f32}));
+ auto reader_without_f64 = GetRecordBatchReader(schema({i64, f32, i32}));
+ auto reader = GetRecordBatchReader(schema({f64, i64, f32, i32}));
+
auto readers = {reader.get(), reader_without_i32.get(), reader_without_f64.get()};
for (auto reader : readers) {
auto source = GetFileSource(reader);
ASSERT_OK_AND_ASSIGN(auto fragment, format_->MakeFragment(*source));
- // NB: projector is applied by the scanner; Fragment does not evaluate it.
- // We will not drop "i32" even though it is not in the projector's schema.
+ // NB: projection is applied by the scanner; FileFragment does not evaluate it so
+ // we will not drop "i32" even though it is not projected since we need it for
+ // filtering
//
// in the case where a file doesn't contain a referenced field, we won't
- // materialize it (the filter/projector will populate it with nulls later)
+ // materialize it as nulls later
Review comment:
The old comment made more sense to me. Doesn't the projection still populate missing fields with null?
##########
File path: cpp/src/arrow/dataset/dataset_test.cc
##########
@@ -794,7 +691,7 @@ TEST(TestDictPartitionColumn, SelectPartitionColumnFilterPhysicalColumn) {
ASSERT_OK_AND_ASSIGN(auto scanner, scan_builder->Finish());
ASSERT_OK_AND_ASSIGN(auto table, scanner->ToTable());
AssertArraysEqual(*table->column(0)->chunk(0),
- *DictArrayFromJSON(partition_field->type(), "[0]", "[\"one\"]"));
+ *ArrayFromJSON(partition_field->type(), R"(["one"])"));
Review comment:
So now that the projection schema is separate from the reader schema, if one wants the output to be dictionary encoded they need to specify so in the projection schema, regardless of the underlying source encoding?
##########
File path: cpp/src/arrow/dataset/expression_test.cc
##########
@@ -1135,5 +1175,36 @@ TEST(Expression, SerializationRoundTrips) {
equal(field_ref("beta"), literal(3.25f))}));
}
+TEST(Projection, AugmentWithNull) {
+ auto just_i32 = ArrayFromJSON(struct_({kBoringSchema->GetFieldByName("i32")}),
+ R"([{"i32": 0}, {"i32": 1}, {"i32": 2}])");
+
+ {
+ ASSERT_OK_AND_ASSIGN(auto proj, project({field_ref("f64"), field_ref("i32")},
+ {"projected double", "projected int"})
+ .Bind(*kBoringSchema));
+
+ auto expected = ArrayFromJSON(
+ struct_({field("projected double", float64()), field("projected int", int32())}),
+ R"([[null, 0], [null, 1], [null, 2]])");
+ ASSERT_OK_AND_ASSIGN(auto actual, ExecuteScalarExpression(proj, just_i32));
+
+ AssertDatumsEqual(Datum(expected), actual);
+ }
+
+ {
+ ASSERT_OK_AND_ASSIGN(
+ auto proj,
+ project({field_ref("f64")}, {"projected double"}).Bind(*kBoringSchema));
+
+ // NB: only a scalar was projected, this is *not* automatically broadcast to an array.
+ ASSERT_OK_AND_ASSIGN(auto expected, StructScalar::Make({MakeNullScalar(float64())},
Review comment:
Maybe it's tested elsewhere but what happens if none of the projected fields have any corresponding value? Will it return a single row of all null? Or zero rows?
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
Rename to `ProjectedFields`?
##########
File path: cpp/src/arrow/compute/kernels/scalar_nested_test.cc
##########
@@ -58,31 +58,48 @@ TEST(Project, Scalar) {
expected.reset(new StructScalar{
{i32, f64, str},
struct_({field("i", i32->type), field("f", f64->type), field("s", str->type)})});
- Project.field_names = {"i", "f", "s"};
- ASSERT_OK_AND_EQ(Datum(expected), Project({i32, f64, str}));
+ ASSERT_OK_AND_EQ(Datum(expected), Project({i32, f64, str}, {"i", "f", "s"}));
// Three field names but one input value
- ASSERT_RAISES(Invalid, Project({str}));
+ ASSERT_RAISES(Invalid, Project({str}, {"i", "f", "s"}));
}
TEST(Project, Array) {
- Project.field_names = {"i", "s"};
+ std::vector<std::string> field_names{"i", "s"};
+
auto i32 = ArrayFromJSON(int32(), "[42, 13, 7]");
auto str = ArrayFromJSON(utf8(), R"(["aa", "aa", "aa"])");
- ASSERT_OK_AND_ASSIGN(Datum expected,
- StructArray::Make({i32, str}, Project.field_names));
+ ASSERT_OK_AND_ASSIGN(Datum expected, StructArray::Make({i32, str}, field_names));
- ASSERT_OK_AND_EQ(expected, Project({i32, str}));
+ ASSERT_OK_AND_EQ(expected, Project({i32, str}, field_names));
// Scalars are broadcast to the length of the arrays
- ASSERT_OK_AND_EQ(expected, Project({i32, MakeScalar("aa")}));
+ ASSERT_OK_AND_EQ(expected, Project({i32, MakeScalar("aa")}, field_names));
// Array length mismatch
- ASSERT_RAISES(Invalid, Project({i32->Slice(1), str}));
+ ASSERT_RAISES(Invalid, Project({i32->Slice(1), str}, field_names));
+}
+
+TEST(Project, NullableMetadataPassedThru) {
+ auto i32 = ArrayFromJSON(int32(), "[42, 13, 7]");
+ auto str = ArrayFromJSON(utf8(), R"(["aa", "aa", "aa"])");
+
+ std::vector<std::string> field_names{"i", "s"};
+ std::vector<bool> nullability{true, false};
+ std::vector<std::shared_ptr<const KeyValueMetadata>> metadata = {
+ key_value_metadata({"a", "b"}, {"ALPHA", "BRAVO"}), nullptr};
+
+ ASSERT_OK_AND_ASSIGN(auto proj,
+ Project({i32, str}, field_names, nullability, metadata));
+
+ AssertTypeEqual(*proj.type(), StructType({
+ field("i", int32(), /*nullable=*/true, metadata[0]),
+ field("s", utf8(), /*nullable=*/false, nullptr),
+ }));
Review comment:
Maybe add a test case where you specify a field is non-nullable but the materialized column has nulls in it (should throw invalid I'd think)
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -115,10 +115,25 @@ struct CompareOptions : public FunctionOptions {
};
struct ARROW_EXPORT ProjectOptions : public FunctionOptions {
- explicit ProjectOptions(std::vector<std::string> n) : field_names(std::move(n)) {}
+ ProjectOptions(std::vector<std::string> n, std::vector<bool> r,
+ std::vector<std::shared_ptr<const KeyValueMetadata>> m)
+ : field_names(std::move(n)),
+ field_nullability(std::move(r)),
Review comment:
Nowhere outside of tests do you specify nullability/metadata. If a customer wanted a field to have a custom metadata couldn't they just add it on themselves after the fact? I suppose you could extend my argument to the field names as well but even SQL allows `as XYZ`.
##########
File path: cpp/src/arrow/dataset/file_ipc_test.cc
##########
@@ -84,9 +85,8 @@ class TestIpcFileFormat : public ArrowIpcWriterMixin {
}
std::unique_ptr<RecordBatchReader> GetRecordBatchReader(
- std::shared_ptr<Schema> schema = nullptr) {
- return MakeGeneratedRecordBatch(schema ? schema : schema_, kBatchSize,
- kBatchRepetitions);
+ std::shared_ptr<Schema> schema) {
Review comment:
Can I fallback to the physical schema if I don't want to specify a projection?
##########
File path: cpp/src/arrow/dataset/scanner_internal.h
##########
@@ -127,5 +139,62 @@ inline Result<ScanTaskIterator> GetScanTaskIterator(
return MakeFlattenIterator(std::move(maybe_scantask_it));
}
+inline Status NestedFieldRefsNotImplemented() {
+ // TODO(ARROW-11259) Several functions (for example, IpcScanTask::Make) assume that
+ // only top level fields will be materialized.
+ return Status::NotImplemented("Nested field references in scans.");
+}
+
+inline Status SetProjection(ScanOptions* options, const Expression& projection) {
+ ARROW_ASSIGN_OR_RAISE(options->projection, projection.Bind(*options->dataset_schema));
+
+ if (options->projection.type()->id() != Type::STRUCT) {
+ return Status::Invalid("Projection ", projection.ToString(),
+ " cannot yield record batches");
+ }
+ options->projected_schema = ::arrow::schema(
+ checked_cast<const StructType&>(*options->projection.type()).fields(),
+ options->dataset_schema->metadata());
+
+ return Status::OK();
+}
+
+inline Status SetProjection(ScanOptions* options, std::vector<Expression> exprs,
+ std::vector<std::string> names) {
+ compute::ProjectOptions project_options{std::move(names)};
+
+ for (size_t i = 0; i < exprs.size(); ++i) {
+ if (auto ref = exprs[i].field_ref()) {
+ if (!ref->name()) return NestedFieldRefsNotImplemented();
+
+ // set metadata and nullability for plain field references
+ ARROW_ASSIGN_OR_RAISE(auto field, ref->GetOne(*options->dataset_schema));
+ project_options.field_nullability[i] = field->nullable();
+ project_options.field_metadata[i] = field->metadata();
+ }
+ }
+
+ return SetProjection(options,
+ call("project", std::move(exprs), std::move(project_options)));
+}
+
+inline Status SetProjection(ScanOptions* options, std::vector<std::string> names) {
+ std::vector<Expression> exprs(names.size());
+ for (size_t i = 0; i < exprs.size(); ++i) {
+ exprs[i] = field_ref(names[i]);
+ }
+ return SetProjection(options, std::move(exprs), std::move(names));
+}
+
+inline Status SetFilter(ScanOptions* options, const Expression& filter) {
+ for (const auto& ref : FieldsInExpression(filter)) {
+ if (!ref.name()) return NestedFieldRefsNotImplemented();
+
+ RETURN_NOT_OK(ref.FindOne(*options->dataset_schema));
Review comment:
Doesn't the filter run against the `projected_schema`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582114128
##########
File path: cpp/src/arrow/dataset/scanner.h
##########
@@ -50,26 +50,16 @@ struct ARROW_DS_EXPORT ScanContext {
std::shared_ptr<internal::TaskGroup> TaskGroup() const;
};
-class ARROW_DS_EXPORT ScanOptions {
- public:
- virtual ~ScanOptions() = default;
-
- static std::shared_ptr<ScanOptions> Make(std::shared_ptr<Schema> schema) {
- return std::shared_ptr<ScanOptions>(new ScanOptions(std::move(schema)));
- }
-
- // Construct a copy of these options with a different schema.
- // The projector will be reconstructed.
- std::shared_ptr<ScanOptions> ReplaceSchema(std::shared_ptr<Schema> schema) const;
-
- // Filter
+struct ARROW_DS_EXPORT ScanOptions {
+ // Filter and projection
Expression filter = literal(true);
+ Expression projection;
- // Schema to which record batches will be reconciled
- const std::shared_ptr<Schema>& schema() const { return projector.schema(); }
+ // Schema with which batches will be read from disk
Review comment:
That is better, will replace
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582990025
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
Ok, now that you are no longer using `projected_schema` I think my comment no longer holds. I assume `FieldsInExpression(projection)` is only going to yield materialized fields? You couldn't have something like (not sure if this is valid SQL or not)...
`SELECT a+b as c, c+d as f`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582080036
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -115,10 +115,25 @@ struct CompareOptions : public FunctionOptions {
};
struct ARROW_EXPORT ProjectOptions : public FunctionOptions {
- explicit ProjectOptions(std::vector<std::string> n) : field_names(std::move(n)) {}
+ ProjectOptions(std::vector<std::string> n, std::vector<bool> r,
+ std::vector<std::shared_ptr<const KeyValueMetadata>> m)
+ : field_names(std::move(n)),
+ field_nullability(std::move(r)),
Review comment:
Currently I only specify nullability and metadata for passthru projected fields https://github.com/bkietz/arrow/blob/1b95d70e4096704c702d1464e93bd218c30498d4/cpp/src/arrow/dataset/scanner_internal.h#L169-L173
For other fields (for example the field resulting from an arithmetic operation) users will need to attach any desired metadata after the scan. I require specifying field names for all projections since the default field name for a column like `SELECT (total - amt) / total` isn't clear to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582069837
##########
File path: cpp/src/arrow/dataset/file_ipc_test.cc
##########
@@ -84,9 +85,8 @@ class TestIpcFileFormat : public ArrowIpcWriterMixin {
}
std::unique_ptr<RecordBatchReader> GetRecordBatchReader(
- std::shared_ptr<Schema> schema = nullptr) {
- return MakeGeneratedRecordBatch(schema ? schema : schema_, kBatchSize,
- kBatchRepetitions);
+ std::shared_ptr<Schema> schema) {
Review comment:
A default projection is provided at a higher level (ScannerBuilder); fragments are not currently designed for direct ergonomic interaction. The default projection includes all columns in the *dataset* schema, which is usually a union of the fragment's physical schemas
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582880766
##########
File path: c_glib/arrow-dataset-glib/scanner.cpp
##########
@@ -265,7 +265,7 @@ gad_scan_options_class_init(GADScanOptionsClass *klass)
gobject_class->set_property = gad_scan_options_set_property;
gobject_class->get_property = gad_scan_options_get_property;
- auto scan_options = arrow::dataset::ScanOptions::Make(arrow::schema({}));
+ auto scan_options = std::make_shared<arrow::dataset::ScanOptions>();
Review comment:
@mrkn https://issues.apache.org/jira/browse/ARROW-11782
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582144270
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -115,10 +115,25 @@ struct CompareOptions : public FunctionOptions {
};
struct ARROW_EXPORT ProjectOptions : public FunctionOptions {
- explicit ProjectOptions(std::vector<std::string> n) : field_names(std::move(n)) {}
+ ProjectOptions(std::vector<std::string> n, std::vector<bool> r,
+ std::vector<std::shared_ptr<const KeyValueMetadata>> m)
+ : field_names(std::move(n)),
+ field_nullability(std::move(r)),
+ field_metadata(std::move(m)) {}
+
+ explicit ProjectOptions(std::vector<std::string> n)
+ : field_names(std::move(n)),
+ field_nullability(field_names.size(), true),
+ field_metadata(field_names.size(), NULLPTR) {}
/// Names for wrapped columns
std::vector<std::string> field_names;
+
+ /// Nullability bits for wrapped columns
+ std::vector<bool> field_nullability;
+
+ /// Metadata attached to wrapped columns
+ std::vector<std::shared_ptr<const KeyValueMetadata>> field_metadata;
Review comment:
That's fine with me; I was just following a perceived convention https://github.com/bkietz/arrow/blob/1b95d70e4096704c702d1464e93bd218c30498d4/cpp/src/arrow/array/array_nested.h#L331
I could also just use a `Field` with a null 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582117038
##########
File path: cpp/src/arrow/dataset/scanner_internal.h
##########
@@ -127,5 +139,62 @@ inline Result<ScanTaskIterator> GetScanTaskIterator(
return MakeFlattenIterator(std::move(maybe_scantask_it));
}
+inline Status NestedFieldRefsNotImplemented() {
Review comment:
Currently I'm using this function and the others below in ScannerBuilder and in several tests to directly set properties of ScanOptions. The duplication is ultimately not necessary and I'll probably either remove ScannerBuilder or reuse it to produce ScanOptions in tests, at which point more of this functionality can migrate to `.cc` files
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582066483
##########
File path: cpp/src/arrow/dataset/dataset_test.cc
##########
@@ -794,7 +691,7 @@ TEST(TestDictPartitionColumn, SelectPartitionColumnFilterPhysicalColumn) {
ASSERT_OK_AND_ASSIGN(auto scanner, scan_builder->Finish());
ASSERT_OK_AND_ASSIGN(auto table, scanner->ToTable());
AssertArraysEqual(*table->column(0)->chunk(0),
- *DictArrayFromJSON(partition_field->type(), "[0]", "[\"one\"]"));
+ *ArrayFromJSON(partition_field->type(), R"(["one"])"));
Review comment:
Dictionary encoding columns in scan is not currently supported; an output column can only be dictionary encoded if it was dictionary encoded in the fragment
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection [WIP]
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#issuecomment-782304056
https://issues.apache.org/jira/browse/ARROW-11174
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582110404
##########
File path: cpp/src/arrow/compute/kernels/scalar_nested_test.cc
##########
@@ -38,18 +39,17 @@ TEST(TestScalarNested, ListValueLength) {
}
struct {
- public:
- Result<Datum> operator()(std::vector<Datum> args) {
- ProjectOptions opts{field_names};
+ template <typename... Options>
+ Result<Datum> operator()(std::vector<Datum> args, std::vector<std::string> field_names,
+ Options... options) {
+ ProjectOptions opts{field_names, options...};
return CallFunction("project", args, &opts);
}
-
- std::vector<std::string> field_names;
} Project;
TEST(Project, Scalar) {
std::shared_ptr<StructScalar> expected(new StructScalar{{}, struct_({})});
- ASSERT_OK_AND_EQ(Datum(expected), Project({}));
+ ASSERT_OK_AND_EQ(Datum(expected), Project({}, {}));
Review comment:
The overload resolution is ambiguous here; I'll make the arguments more explicit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r583924716
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
Correct, that's not supported.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582376636
##########
File path: cpp/src/arrow/dataset/expression_test.cc
##########
@@ -1135,5 +1175,36 @@ TEST(Expression, SerializationRoundTrips) {
equal(field_ref("beta"), literal(3.25f))}));
}
+TEST(Projection, AugmentWithNull) {
+ auto just_i32 = ArrayFromJSON(struct_({kBoringSchema->GetFieldByName("i32")}),
+ R"([{"i32": 0}, {"i32": 1}, {"i32": 2}])");
+
+ {
+ ASSERT_OK_AND_ASSIGN(auto proj, project({field_ref("f64"), field_ref("i32")},
+ {"projected double", "projected int"})
+ .Bind(*kBoringSchema));
+
+ auto expected = ArrayFromJSON(
+ struct_({field("projected double", float64()), field("projected int", int32())}),
+ R"([[null, 0], [null, 1], [null, 2]])");
+ ASSERT_OK_AND_ASSIGN(auto actual, ExecuteScalarExpression(proj, just_i32));
+
+ AssertDatumsEqual(Datum(expected), actual);
+ }
+
+ {
+ ASSERT_OK_AND_ASSIGN(
+ auto proj,
+ project({field_ref("f64")}, {"projected double"}).Bind(*kBoringSchema));
+
+ // NB: only a scalar was projected, this is *not* automatically broadcast to an array.
+ ASSERT_OK_AND_ASSIGN(auto expected, StructScalar::Make({MakeNullScalar(float64())},
Review comment:
Consider a dataset with tables...
```
A,B
1,"x"
2,"x"
A
3
4
```
Then the query is `SELECT B,'1' as C FROM dataset`
This would output...
```
B,C
"X",1
"X",1
NULL,1
```
...right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582066483
##########
File path: cpp/src/arrow/dataset/dataset_test.cc
##########
@@ -794,7 +691,7 @@ TEST(TestDictPartitionColumn, SelectPartitionColumnFilterPhysicalColumn) {
ASSERT_OK_AND_ASSIGN(auto scanner, scan_builder->Finish());
ASSERT_OK_AND_ASSIGN(auto table, scanner->ToTable());
AssertArraysEqual(*table->column(0)->chunk(0),
- *DictArrayFromJSON(partition_field->type(), "[0]", "[\"one\"]"));
+ *ArrayFromJSON(partition_field->type(), R"(["one"])"));
Review comment:
Dictionary encoding columns in scan is not currently supported; an output column can only be dictionary encoded if it was dictionary encoded on disk.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#issuecomment-785854425
@nealrichardson thanks for testing! I'll add an analogous c++ test and get it passing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582072221
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
For `project({add(field_ref("a"), field_ref("b"))}, {"a_plus_b"})` (SQL: `SELECT a + b as a_plus_b`) the materialized fields would be `{a,b}` while the projected fields would be `{a_plus_b}`. I'll add a long-form docstring clarifying 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582024887
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -115,10 +115,25 @@ struct CompareOptions : public FunctionOptions {
};
struct ARROW_EXPORT ProjectOptions : public FunctionOptions {
- explicit ProjectOptions(std::vector<std::string> n) : field_names(std::move(n)) {}
+ ProjectOptions(std::vector<std::string> n, std::vector<bool> r,
+ std::vector<std::shared_ptr<const KeyValueMetadata>> m)
+ : field_names(std::move(n)),
+ field_nullability(std::move(r)),
+ field_metadata(std::move(m)) {}
+
+ explicit ProjectOptions(std::vector<std::string> n)
+ : field_names(std::move(n)),
+ field_nullability(field_names.size(), true),
+ field_metadata(field_names.size(), NULLPTR) {}
/// Names for wrapped columns
std::vector<std::string> field_names;
+
+ /// Nullability bits for wrapped columns
+ std::vector<bool> field_nullability;
+
+ /// Metadata attached to wrapped columns
+ std::vector<std::shared_ptr<const KeyValueMetadata>> field_metadata;
Review comment:
Would it make sense to store a single vector, e.g.:
```c++
struct ProjectField {
std::string name;
bool nullable;
std::shared_ptr<const KeyValueMetadata> metadata;
ProjectField(std::string name, bool nullable=false,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR)
: name(std::move(name)), nullable(nullable), metadata(std::move(metadata)) {}
};
std::vector<ProjectField> fields_;
```
##########
File path: cpp/src/arrow/dataset/scanner.h
##########
@@ -210,11 +197,23 @@ class ARROW_DS_EXPORT ScannerBuilder {
/// Schema.
Status Project(std::vector<std::string> columns);
+ /// \brief Set expressions which will be evaluated to produce the materialized columns.
+ ///
+ /// Columns which are not referenced may not be loaded from disk.
Review comment:
"from fragments"?
##########
File path: cpp/src/arrow/dataset/scanner_internal.h
##########
@@ -127,5 +139,62 @@ inline Result<ScanTaskIterator> GetScanTaskIterator(
return MakeFlattenIterator(std::move(maybe_scantask_it));
}
+inline Status NestedFieldRefsNotImplemented() {
Review comment:
Is there a reason for making everything inline in this file (rather than define the functions in a `.cc` file)?
##########
File path: cpp/src/arrow/dataset/scanner.h
##########
@@ -50,26 +50,16 @@ struct ARROW_DS_EXPORT ScanContext {
std::shared_ptr<internal::TaskGroup> TaskGroup() const;
};
-class ARROW_DS_EXPORT ScanOptions {
- public:
- virtual ~ScanOptions() = default;
-
- static std::shared_ptr<ScanOptions> Make(std::shared_ptr<Schema> schema) {
- return std::shared_ptr<ScanOptions>(new ScanOptions(std::move(schema)));
- }
-
- // Construct a copy of these options with a different schema.
- // The projector will be reconstructed.
- std::shared_ptr<ScanOptions> ReplaceSchema(std::shared_ptr<Schema> schema) const;
-
- // Filter
+struct ARROW_DS_EXPORT ScanOptions {
+ // Filter and projection
Expression filter = literal(true);
+ Expression projection;
- // Schema to which record batches will be reconciled
- const std::shared_ptr<Schema>& schema() const { return projector.schema(); }
+ // Schema with which batches will be read from disk
Review comment:
Better "from fragments"?
##########
File path: cpp/src/arrow/dataset/scanner.h
##########
@@ -210,11 +197,23 @@ class ARROW_DS_EXPORT ScannerBuilder {
/// Schema.
Status Project(std::vector<std::string> columns);
+ /// \brief Set expressions which will be evaluated to produce the materialized columns.
+ ///
+ /// Columns which are not referenced may not be loaded from disk.
+ ///
+ /// \param[in] exprs expressions to evaluate to produce columns.
+ /// \param[in] names list of names for the resulting columns.
+ ///
+ /// \return Failure if any referenced column does not exists in the dataset's
+ /// Schema.
Review comment:
"physical schema"?
##########
File path: cpp/src/arrow/dataset/scanner.h
##########
@@ -50,26 +50,16 @@ struct ARROW_DS_EXPORT ScanContext {
std::shared_ptr<internal::TaskGroup> TaskGroup() const;
};
-class ARROW_DS_EXPORT ScanOptions {
- public:
- virtual ~ScanOptions() = default;
-
- static std::shared_ptr<ScanOptions> Make(std::shared_ptr<Schema> schema) {
- return std::shared_ptr<ScanOptions>(new ScanOptions(std::move(schema)));
- }
-
- // Construct a copy of these options with a different schema.
- // The projector will be reconstructed.
- std::shared_ptr<ScanOptions> ReplaceSchema(std::shared_ptr<Schema> schema) const;
-
- // Filter
+struct ARROW_DS_EXPORT ScanOptions {
+ // Filter and projection
Expression filter = literal(true);
+ Expression projection;
- // Schema to which record batches will be reconciled
- const std::shared_ptr<Schema>& schema() const { return projector.schema(); }
+ // Schema with which batches will be read from disk
+ std::shared_ptr<Schema> dataset_schema;
Review comment:
So this is the physical schema?
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -130,12 +139,16 @@ TEST_F(TestCsvFileFormat, IsSupported) {
ASSERT_OK_AND_ASSIGN(supported, format_->IsSupported(*source));
ASSERT_EQ(supported, false);
- source = GetFileSource();
+ source = GetFileSource(R"(f64
+1.0
+
+N/A
+2)");
ASSERT_OK_AND_ASSIGN(supported, format_->IsSupported(*source));
EXPECT_EQ(supported, true);
}
-TEST_F(TestCsvFileFormat, DISABLED_NonMaterializedFieldWithDifferingTypeFromInferred) {
+TEST_F(TestCsvFileFormat, NonMaterializedFieldWithDifferingTypeFromInferred) {
Review comment:
AFAICT, it means the `str` column from the CSV file is part of the physical schema, but not the dataset schema (indeed the CSV scanner should not "materialize" it as a Arrow column).
@bkietz Should you add a check for the physical schema here?
##########
File path: cpp/src/arrow/compute/kernels/scalar_nested_test.cc
##########
@@ -38,18 +39,17 @@ TEST(TestScalarNested, ListValueLength) {
}
struct {
- public:
- Result<Datum> operator()(std::vector<Datum> args) {
- ProjectOptions opts{field_names};
+ template <typename... Options>
+ Result<Datum> operator()(std::vector<Datum> args, std::vector<std::string> field_names,
+ Options... options) {
+ ProjectOptions opts{field_names, options...};
return CallFunction("project", args, &opts);
}
-
- std::vector<std::string> field_names;
} Project;
TEST(Project, Scalar) {
std::shared_ptr<StructScalar> expected(new StructScalar{{}, struct_({})});
- ASSERT_OK_AND_EQ(Datum(expected), Project({}));
+ ASSERT_OK_AND_EQ(Datum(expected), Project({}, {}));
Review comment:
Is the second argument an empty ProjectOptions? Something else?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582113830
##########
File path: cpp/src/arrow/dataset/scanner.h
##########
@@ -50,26 +50,16 @@ struct ARROW_DS_EXPORT ScanContext {
std::shared_ptr<internal::TaskGroup> TaskGroup() const;
};
-class ARROW_DS_EXPORT ScanOptions {
- public:
- virtual ~ScanOptions() = default;
-
- static std::shared_ptr<ScanOptions> Make(std::shared_ptr<Schema> schema) {
- return std::shared_ptr<ScanOptions>(new ScanOptions(std::move(schema)));
- }
-
- // Construct a copy of these options with a different schema.
- // The projector will be reconstructed.
- std::shared_ptr<ScanOptions> ReplaceSchema(std::shared_ptr<Schema> schema) const;
-
- // Filter
+struct ARROW_DS_EXPORT ScanOptions {
+ // Filter and projection
Expression filter = literal(true);
+ Expression projection;
- // Schema to which record batches will be reconciled
- const std::shared_ptr<Schema>& schema() const { return projector.schema(); }
+ // Schema with which batches will be read from disk
+ std::shared_ptr<Schema> dataset_schema;
Review comment:
It is not. The physical schemas of fragments are allowed to diverge from the dataset schema (missing columns, eventually an integer column might promote from int32 in older fragments to int64 in newer fragments, etc). The dataset schema is a schema to which those physical schemas can be reconciled
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582064969
##########
File path: cpp/src/arrow/dataset/expression_test.cc
##########
@@ -1135,5 +1175,36 @@ TEST(Expression, SerializationRoundTrips) {
equal(field_ref("beta"), literal(3.25f))}));
}
+TEST(Projection, AugmentWithNull) {
+ auto just_i32 = ArrayFromJSON(struct_({kBoringSchema->GetFieldByName("i32")}),
+ R"([{"i32": 0}, {"i32": 1}, {"i32": 2}])");
+
+ {
+ ASSERT_OK_AND_ASSIGN(auto proj, project({field_ref("f64"), field_ref("i32")},
+ {"projected double", "projected int"})
+ .Bind(*kBoringSchema));
+
+ auto expected = ArrayFromJSON(
+ struct_({field("projected double", float64()), field("projected int", int32())}),
+ R"([[null, 0], [null, 1], [null, 2]])");
+ ASSERT_OK_AND_ASSIGN(auto actual, ExecuteScalarExpression(proj, just_i32));
+
+ AssertDatumsEqual(Datum(expected), actual);
+ }
+
+ {
+ ASSERT_OK_AND_ASSIGN(
+ auto proj,
+ project({field_ref("f64")}, {"projected double"}).Bind(*kBoringSchema));
+
+ // NB: only a scalar was projected, this is *not* automatically broadcast to an array.
+ ASSERT_OK_AND_ASSIGN(auto expected, StructScalar::Make({MakeNullScalar(float64())},
Review comment:
This test illustrates that case: `f64` is not present in the input batch, and the result of such projection is a StructScalar whose fields are null
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582336228
##########
File path: cpp/src/arrow/dataset/scanner_internal.h
##########
@@ -127,5 +139,62 @@ inline Result<ScanTaskIterator> GetScanTaskIterator(
return MakeFlattenIterator(std::move(maybe_scantask_it));
}
+inline Status NestedFieldRefsNotImplemented() {
+ // TODO(ARROW-11259) Several functions (for example, IpcScanTask::Make) assume that
+ // only top level fields will be materialized.
+ return Status::NotImplemented("Nested field references in scans.");
+}
+
+inline Status SetProjection(ScanOptions* options, const Expression& projection) {
+ ARROW_ASSIGN_OR_RAISE(options->projection, projection.Bind(*options->dataset_schema));
+
+ if (options->projection.type()->id() != Type::STRUCT) {
+ return Status::Invalid("Projection ", projection.ToString(),
+ " cannot yield record batches");
+ }
+ options->projected_schema = ::arrow::schema(
+ checked_cast<const StructType&>(*options->projection.type()).fields(),
+ options->dataset_schema->metadata());
+
+ return Status::OK();
+}
+
+inline Status SetProjection(ScanOptions* options, std::vector<Expression> exprs,
+ std::vector<std::string> names) {
+ compute::ProjectOptions project_options{std::move(names)};
+
+ for (size_t i = 0; i < exprs.size(); ++i) {
+ if (auto ref = exprs[i].field_ref()) {
+ if (!ref->name()) return NestedFieldRefsNotImplemented();
+
+ // set metadata and nullability for plain field references
+ ARROW_ASSIGN_OR_RAISE(auto field, ref->GetOne(*options->dataset_schema));
+ project_options.field_nullability[i] = field->nullable();
+ project_options.field_metadata[i] = field->metadata();
+ }
+ }
+
+ return SetProjection(options,
+ call("project", std::move(exprs), std::move(project_options)));
+}
+
+inline Status SetProjection(ScanOptions* options, std::vector<std::string> names) {
+ std::vector<Expression> exprs(names.size());
+ for (size_t i = 0; i < exprs.size(); ++i) {
+ exprs[i] = field_ref(names[i]);
+ }
+ return SetProjection(options, std::move(exprs), std::move(names));
+}
+
+inline Status SetFilter(ScanOptions* options, const Expression& filter) {
+ for (const auto& ref : FieldsInExpression(filter)) {
+ if (!ref.name()) return NestedFieldRefsNotImplemented();
+
+ RETURN_NOT_OK(ref.FindOne(*options->dataset_schema));
Review comment:
No: both the filter and the projection will run against the dataset schema
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582331580
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -130,12 +139,16 @@ TEST_F(TestCsvFileFormat, IsSupported) {
ASSERT_OK_AND_ASSIGN(supported, format_->IsSupported(*source));
ASSERT_EQ(supported, false);
- source = GetFileSource();
+ source = GetFileSource(R"(f64
+1.0
+
+N/A
+2)");
ASSERT_OK_AND_ASSIGN(supported, format_->IsSupported(*source));
EXPECT_EQ(supported, true);
}
-TEST_F(TestCsvFileFormat, DISABLED_NonMaterializedFieldWithDifferingTypeFromInferred) {
+TEST_F(TestCsvFileFormat, NonMaterializedFieldWithDifferingTypeFromInferred) {
Review comment:
This should have read "NonProjected" rather than "NonMaterialized", sorry for the confusion and I'll amend. `f64` will be materialized (since we need to filter on it) but is not projected (so it won't appear in scanned batches).
The physical schema of the batch will have the inferred type for each column, I'll add an assertion of this.
The key point of this test case is that we request `f64` be read *as a string*, rather than a double. This is uncommon and only possible due to the textual nature of CSV, but it is not illegal. If the csv reader fails to respect the stated dataset schema this can break filter expressions in surprising ways.
For example it might erroneously consider only the projected schema and revert to inferring the type of a non projected field which would result in a comparison between a double and a string column.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #9532:
URL: https://github.com/apache/arrow/pull/9532
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582190291
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
So is the `projected_schema` in that case `schema({"a_plus_b"})` or `schema({"a", "b"})`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582117582
##########
File path: cpp/src/arrow/compute/kernels/scalar_nested_test.cc
##########
@@ -58,31 +58,48 @@ TEST(Project, Scalar) {
expected.reset(new StructScalar{
{i32, f64, str},
struct_({field("i", i32->type), field("f", f64->type), field("s", str->type)})});
- Project.field_names = {"i", "f", "s"};
- ASSERT_OK_AND_EQ(Datum(expected), Project({i32, f64, str}));
+ ASSERT_OK_AND_EQ(Datum(expected), Project({i32, f64, str}, {"i", "f", "s"}));
// Three field names but one input value
- ASSERT_RAISES(Invalid, Project({str}));
+ ASSERT_RAISES(Invalid, Project({str}, {"i", "f", "s"}));
}
TEST(Project, Array) {
- Project.field_names = {"i", "s"};
+ std::vector<std::string> field_names{"i", "s"};
+
auto i32 = ArrayFromJSON(int32(), "[42, 13, 7]");
auto str = ArrayFromJSON(utf8(), R"(["aa", "aa", "aa"])");
- ASSERT_OK_AND_ASSIGN(Datum expected,
- StructArray::Make({i32, str}, Project.field_names));
+ ASSERT_OK_AND_ASSIGN(Datum expected, StructArray::Make({i32, str}, field_names));
- ASSERT_OK_AND_EQ(expected, Project({i32, str}));
+ ASSERT_OK_AND_EQ(expected, Project({i32, str}, field_names));
// Scalars are broadcast to the length of the arrays
- ASSERT_OK_AND_EQ(expected, Project({i32, MakeScalar("aa")}));
+ ASSERT_OK_AND_EQ(expected, Project({i32, MakeScalar("aa")}, field_names));
// Array length mismatch
- ASSERT_RAISES(Invalid, Project({i32->Slice(1), str}));
+ ASSERT_RAISES(Invalid, Project({i32->Slice(1), str}, field_names));
+}
+
+TEST(Project, NullableMetadataPassedThru) {
+ auto i32 = ArrayFromJSON(int32(), "[42, 13, 7]");
+ auto str = ArrayFromJSON(utf8(), R"(["aa", "aa", "aa"])");
+
+ std::vector<std::string> field_names{"i", "s"};
+ std::vector<bool> nullability{true, false};
+ std::vector<std::shared_ptr<const KeyValueMetadata>> metadata = {
+ key_value_metadata({"a", "b"}, {"ALPHA", "BRAVO"}), nullptr};
+
+ ASSERT_OK_AND_ASSIGN(auto proj,
+ Project({i32, str}, field_names, nullability, metadata));
+
+ AssertTypeEqual(*proj.type(), StructType({
+ field("i", int32(), /*nullable=*/true, metadata[0]),
+ field("s", utf8(), /*nullable=*/false, nullptr),
+ }));
Review comment:
Will do
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582813969
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -33,21 +34,10 @@
namespace arrow {
namespace dataset {
-ScanOptions::ScanOptions(std::shared_ptr<Schema> schema)
- : projector(RecordBatchProjector(std::move(schema))) {}
-
-std::shared_ptr<ScanOptions> ScanOptions::ReplaceSchema(
- std::shared_ptr<Schema> schema) const {
- auto copy = ScanOptions::Make(std::move(schema));
- copy->filter = filter;
- copy->batch_size = batch_size;
- return copy;
-}
-
std::vector<std::string> ScanOptions::MaterializedFields() const {
Review comment:
```python
assert materialized_schema.names == ['a', 'b']
assert projected_schema.names == ['a_plus_b']
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582900019
##########
File path: c_glib/arrow-dataset-glib/scanner.cpp
##########
@@ -265,7 +265,7 @@ gad_scan_options_class_init(GADScanOptionsClass *klass)
gobject_class->set_property = gad_scan_options_set_property;
gobject_class->get_property = gad_scan_options_get_property;
- auto scan_options = arrow::dataset::ScanOptions::Make(arrow::schema({}));
+ auto scan_options = std::make_shared<arrow::dataset::ScanOptions>();
Review comment:
OK, I will create a new pull-request to remove the bindings of these internal classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #9532: ARROW-11174: [C++][Dataset] Make expressions available to projection
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #9532:
URL: https://github.com/apache/arrow/pull/9532#discussion_r582881412
##########
File path: java/dataset/src/test/java/org/apache/arrow/dataset/jni/TestReservationListener.java
##########
@@ -58,9 +53,8 @@ public void testDirectReservationListener() throws Exception {
AutoCloseables.close(datum);
AutoCloseables.close(pool);
long finalReservation = DirectReservationListener.instance().getCurrentDirectMemReservation();
- final long expected_diff = DEFAULT_NATIVE_MEMORY_POOL_BLOCK_SIZE;
- Assert.assertEquals(expected_diff, reservation - initReservation);
- Assert.assertEquals(-expected_diff, finalReservation - reservation);
+ Assert.assertTrue(reservation >= initReservation);
+ Assert.assertTrue(finalReservation == initReservation);
Review comment:
@zhztheplayer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org