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?

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

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?

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

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?

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

What does NonMaterialized mean in this context? Maybe ProjectedFieldDifferingTypeFromInferred?

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?

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.

The old comment made more sense to me. Doesn't the projection still populate missing fields with null?

Rename to ProjectedFields?

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

So is the projected_schema in that case schema({"a_plus_b"}) or schema({"a", "b"})?

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

Correct, FieldsInExpression(projection) will yield only materialized fields. That SQL's not supported; only fields in the dataset schema may be referenced

Doesn't the filter run against the projected_schema?

No: both the filter and the projection will run against the dataset schema

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.

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.

Would it make sense to store a single vector, e.g.:

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

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

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?

So this is the physical schema?

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

"physical schema"?

Is there a reason for making everything inline in this file (rather than define the functions in a .cc file)?

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

@mrkn https://issues.apache.org/jira/browse/ARROW-11782

OK, I will create a new pull-request to remove the bindings of these internal classes.

@zhztheplayer