Patched DataFusion, Oct 30, 2024

[wiedld]

This takes us up to Oct-30 SHA 4975829

It advances DF by adding these oct-15 to oct-30 commits: 399e840...influxdata:arrow-datafusion:497582906e826d3045e2c7b9d4ddb44ce6d42adf

NOTE: this does not include:

• the SPM update, which is the last change on oct-30: f23360f
• the internal use of view types for array record batch processing: 2d7892b

Patches applied

• Patches already fixed in future upstream commits:

  • Nov-3 = apache@85f92ef
  • Nov-11 = apache@be19afc
  • clippy updates (presumably these are a bunch of separate upstream commits)
  • SanityCheckPlan
    • I'm not sure when we can stop skipping the SanityCheckPlan. I'm unclear how it relates to the additional sort pushdown fixes, including a recent fix done 3 weeks ago: [bug]: Fix wrong order by removal from plan apache/datafusion#13497

• Patches still not fixed:

  • commit reverted due to CPU spinning bug (new)
  • This empty projection patch, since I only ever found these plans in iox-created influqxl LP.
    • patch: cb44b88
    • iox issue: https://github.com/influxdata/influxdb_iox/issues/12407

• Patches specific to our DF branch (no need for upstream fix):

  • bit_length(utf8view) patch: 90423e2
  • we are keeping in the warn logging for the schema mismatch. (Upstream only returns error, or not, based on schema misalignment): our patch 69d6118
  • we are using an extra few bytes for the GroupedHashAggregateStream: 50e1209

• tech debt to be fixed:

  • we are keeping in the warn logging for the schema mismatch. (Upstream only returns error, or not, based on schema misalignment): our patch 69d6118
  • I reverted/excluded the commit to turn on view types by default (for arrow processing):
    • we had errors for iox tests with existing parquet, which contained string value fields.
      • e.g. end_to_end_cases::cli::write_and_query, end_to_end_cases::cli::table::create_write_and_query.
      • The exact error thrown was an iox schema check. Even if this was updated to accept view types as valid => we had arrow cast errors where it kept attempting ot cast to string array (not view).
        * I can guess this is due to our various iox codepoints where we map String fields to arrow's Utf8. I would rather not muck around with that, unless it's done separately.
        * Additionally, setting schema_force_view_types=false feels fragile since it's nested in a various options. To be on the safe side, I would rather revert this commit for now.

[wiedld]

• we are using an extra few bytes for the GroupedHashAggregateStream: 50e1209

@alamb note that this is a new commit.

.

• This empty projection patch, since I only ever found these plans in iox-created influqxl LP.
  • patch: cb44b88
  • iox issue: https://github.com/influxdata/influxdb_iox/issues/12407

@alamb note that this is a newly re-written commit, now that we advanced to the latest (on DF main) version of union_schema(). This is the remaining bit we patch to handle our iox-specific issue.

[wiedld]

In order to get iox CI passing, I decided to back out the commit to turn on view types. See this PR description.

In brief, existing parquet with string fields was failing in our iox tests. IMO, we should try making this change separately; not with 2 weeks of other DF changes. What do you think @alamb ?

[alamb]

I reverted/excluded the commit to turn on view types by default (for arrow processing):

the internal use of view types for array record batch processing: 2d7892b

I recommend:

1. Include the code change

2. Changing the influxdb_iox config to turn off the view types by default (e.g. https://github.com/influxdata/influxdb_iox/blob/496b298676097aaf0eb52f67957809f147ed4a56/datafusion_util/src/config.rs#L25-L29). Specifically set datafusion.execution.parquet.schema_force_view_types to false

3. File a ticket tracking the work needed to turn it on by default in IOx

The rationale to do this is to keep our fork as close as possible to DataFusion main (and we can then work on using the default value of the datafusion.execution.parquet.schema_force_view_types setting without having to change DataFusion versions too

(we can also do this in a follow on PR)

[wiedld]

I recommend:

1. Include the code change

2. Changing the influxdb_iox config to turn off the view types by default (e.g. https://github.com/influxdata/influxdb_iox/blob/496b298676097aaf0eb52f67957809f147ed4a56/datafusion_util/src/config.rs#L25-L29). Specifically set `datafusion.execution.parquet.schema_force_view_types` to `false`

3. File a ticket tracking the work needed to turn it on by default in IOx

@alamb -- Agreed that we should file this as a tech debt ticket in iox, to get us onto view types. However, I'm not sure whether our "tech debt state" should be the revert upstream commit -- or figuring out how to turn it off via the config.

Yesterday I tried turning off via the config first, but was still getting CI errors. Here is an example WIP showing that we need more than just setting the config. (Or maybe I'm missing another point of where the config should be set? 🤔 )

Specifically, the CI errors were the bits mentioned in the PR description. It was still attempting to use utf8view types and failing the iox schema check. I could modify the check, but then it failed on the various iox-type to arrow-type mappings. Etc. It became clear that this was a larger thing, to be done in it's own (atomically revertable) pr IMO.

[wiedld]

Specifically, the CI errors were the bits mentioned in the PR description. It was still attempting to use utf8view types and failing the iox schema check. I could modify the check, but then it failed on the various iox-type to arrow-type mappings. Etc. It became clear that this was a larger thing, to be done in it's own (atomically revertable) pr IMO.

I just realized that this^^ observation may mean that we have an upstream DF bug. Since when turning off view types, we were still getting view types. 🤦🏼‍♀️ (Or I'm missing a config setting somewhere in iox.)

[wiedld]

And another thing that bugs me in upstream. There are a whole bunch of duplicate places where the ParquetTableOptions are provided in the session. Which is why I'm not sure if I'm (a) still missing a place to set the config (even after changing all of these places), or (b) there is a bug in upstream where DF is still internally trying to use view types when we turned it off. 🤷🏼‍♀️

[alamb]

And another thing that bugs me in upstream. There are a whole bunch of duplicate places where the ParquetTableOptions are provided in the session.

What I normally do in this case is go start making small PRs upstream to make the situation better

Which is why I'm not sure if I'm (a) still missing a place to set the config (even after changing all of these places), or (b) there is a bug in upstream where DF is still internally trying to use view types when we turned it off. 🤷🏼‍♀️

I likewise don't know -- can you give me a hint about how I can help you debug this (aka how do I reproduce the issue?)