fix: preserve projection metadata during optimization

[gene-bordegaray]

There were four ways metadata could disappear.

1. Removing a metadata-only identity projection

Consider:

ProjectionExec: i@0 AS i
  output field metadata = {"event_field": "true"}

  DataSourceExec: i
    field metadata = {}

The check to remove the projection asked:

• Is every expression a column?
• Does column 0 remain column 0?
• Does the alias match the column name?
• Does the number of columns match?

All answers yes so optimizer removed projection:

DataSourceExec: i
 metadata = {}

Metadata lost.

2 Collapsing across a metadata boundary

Consider:

ProjectionExec: arrow_metadata(i, 'event_field') AS metadata

  ProjectionExec: i@0 AS i
    output metadata = {"event_field": "true"}

    DataSourceExec: i
      metadata = {}

The correct result is true.

The previous projection colapse logic would substitute the outer expression through the inner projection:

ProjectionExec: arrow_metadata(i, 'event_field') AS metadata

 DataSourceExec: i
   metadata = {}

Now the func sees the scan field instead of the inner projection field giving use result as NULL now.

3 Rebuilding a projection with a new child

Some optimizer paths replace the child of a projection:

Old:
ProjectionExec(metadata={"key": "value"})

 OldChild

---

New:
ProjectionExec(...?)

 NewChild

The previous make_with_child implementation did this:

ProjectionExec::try_new(projection.expr().to_vec(), new_child)

where try_new derives the output schema from the expressions and new child so it woudlnt retain metadata from the original projection.

4 Cast target metadata lost before optimization

This one was a little confusing because main passed the UUID metadata test, but the first version of this PR did not (@gabotechs this is what you called out)

Basically a cast can have an explicit target field with metadata. For example, the UUID type planner produces:

FixedSizeBinary(16)
  metadata = {"ARROW:extension:name": "arrow.uuid"}

But logical cast schema only used the target data type when deriving and kept th source metadata:

Source field:
  raw: FixedSizeBinary(16)
  metadata: {}

Cast target:
  FixedSizeBinary(16)
  metadata: {"ARROW:extension:name": "arrow.uuid"}

Derived cast output:
  FixedSizeBinary(16)
  metadata: {}

This appeared in CI when common sub-expr elimination extracts a repeated cast into
its own projection:

ProjectionExec: arrow_metadata(__common_expr_1, 'ARROW:extension:name')

 ProjectionExec: CAST(raw AS UUID) AS __common_expr_1

The inner projection was initially created with incorrect empty metadata, so the physical optimizer rebuilt that projection and rederived its schema so isthe was accidentally repairing the logical schema bug.

Once this PR started preserving projection metadata correctly had this pop up this other bug.

5. Physical-plan serialization

Before ProjectionExecNode serialized:

PhysicalPlanNode input = 1;
repeated PhysicalExprNode expr = 2;
repeated string expr_name = 3;

On decode, DataFusion reconstructed the projection which is fine when the complete output schema can be derivde from the expression and input but metadata cannot be:
Given:

Input field:
  value: Int32
  metadata: {}

Expression: value@0 AS value

---

derives...

---

Output field:
  value: Int32
  metadata: {}

Receiving process gets...

---

ProjectionExec
  output schema:
    value: Int32
    field metadata: {}
    schema metadata: {}

---

So then I solve the optimizer bugs in this PR but not the codex one and will create a follow up for that.

TODO: Create follow up for codec / serialization

[gabotechs]

Good catch @gene-bordegaray! just to give more context, we were bitten by this in our system while upgrading.

Just left a suggestion for relaxing the requirements, but otherwise LGTM.

[gabotechs]

This might be putting more restrictions than just metadata equality. It might be fine, but if we want to play it safe it could be better to just do && projection.schema().metadata() == projection.input().schema().metadata()

[gene-bordegaray]

We need to check full schema because even if the schema metadata is equal things like the field metadata might not be thus we nee to check this as well.

I don't see anything in the schema which would be overestricting this. I may be missing something though

[gabotechs]

Pretty much the order of columns. I bet that's why the current checks are like they are right now.

I think it's fine though, if this becomes too restrictive it will start popping up in tests

[gabotechs]

🤔 there seems to be a CI failure:

1. query result mismatch:
[SQL] SELECT
    CAST(raw AS UUID),
    arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name')
FROM (
    VALUES (
        arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
    )
) AS uuids(raw);
[Diff] (-expected|+actual)
-   00010203040506070809000102030506 arrow.uuid
+   00010203040506070809000102030506 NULL
at /home/runner/work/datafusion/datafusion/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:36

Do you think it's related to this change?

[gene-bordegaray]

Do you think it's related to this change?

looking into

[gene-bordegaray]

Do you think it's related to this change?

Found issues, this is a bit more involved than I was hoping. Will the variants with the fix

[gene-bordegaray]

@gabotechs ok I figured out what was going on and documented it in the PR description. There is also another bug in the codec / serialization where we need to serialize metadata. I am not solving that in this PR to keep scoped / tracked. I will crete issue for this tmrw or you can if you would like 👍

[gene-bordegaray]

this is also a correctenss issue / regression in 55 so I can note this in the minor version bump