Set projection before configuring the source #14685

blaginin · 2025-02-15T21:25:26Z

Which issue does this PR close?

Rationale for this change

FileScanConfig::new now also configures source, but this currently without projection set before. And so when source.with_statistics is called, it is passed with yet empty projected_statistics

What changes are included in this PR?

Setting projection before configuring the source... but should the old method exist?

Are these changes tested?

Extended a test to check stats

Are there any user-facing changes?

No

mertak-synnada · 2025-02-17T10:25:21Z

Hello, thanks for reproducing the bug and the PR!
I think the problem is not with the call order of the functions but with missing behavior. How about changing the with_projection method to update statistics as well? Just like we do in the with_source.

let (
    _projected_schema,
    _constraints,
    projected_statistics,
    _projected_output_ordering,
) = self.project();
self.source = source.with_statistics(projected_statistics);

blaginin · 2025-02-17T20:20:39Z

Thanks for checking! I also thought that, but couldn't do because project relies on other fields to be set. You can see that for example in partition_column_projector test, where we call with_projection before with_table_partition_cols, so recomputing on isn't feasible

I just pushed explicit refresh, but maybe it's time to introduce FileScanConfigBuilder (in a separate pr)?

mertak-synnada · 2025-02-19T07:04:43Z

Yes, I agree with the builder approach, it might be helpful for such cases. However, I'm a bit concerned about with the explicit approach, since it might be forgotten. It looks like this should be in the FileScanConfigs responsibility, that's why I think we should call the source updater method implicitly on both with_projection and with_table_partition_cols

blaginin · 2025-02-19T21:13:32Z

Fair, let's do it that way as a hotfix and properly solve with the builder then 🤝

milenkovicm · 2025-02-24T22:42:26Z

Would it make sense to add a test which would cover plan round trip?

xudong963 · 2025-02-25T15:01:47Z

datafusion/core/src/datasource/physical_plan/file_scan_config.rs

+        let (
+            _projected_schema,
+            _constraints,
+            projected_statistics,
+            _projected_output_ordering,
+        ) = self.project();
+
+        self.source = self.source.with_statistics(projected_statistics);
        self


How about

let source = self.source.clone(); self.with_source(source)

berkaysynnada

Thank you @blaginin, the current state is LGTM. Could we add a test to cover #14679?

PTAL @mertak-synnada

berkaysynnada · 2025-02-26T06:48:58Z

datafusion/core/src/datasource/physical_plan/file_scan_config.rs

+
+        if max_projection_column
+            >= self.file_schema.fields().len() + self.table_partition_cols.len()
+        {


How is this case possible? it seems not obvious to me

removed this now. but answering to the question - this could happen if projection set but table_partition_cols isn't yet set (or vice versa).

This whole logic should be much cleaner when we switch to the builder approach (I want to do a PR on top after this one is merged)

alamb · 2025-02-26T11:59:12Z

I am pretty sure I am hitting this issue while upgrading to delta.rs as well in

chore: upgrade to DataFusion 46.0.0 (WORK IN PROGRESS) delta-io/delta-rs#3261
I will help shortly

alamb · 2025-02-26T14:46:09Z

datafusion/core/src/datasource/physical_plan/file_scan_config.rs

+            _projected_output_ordering,
+        ) = self.project();
+
+        self.source = self.source.with_statistics(projected_statistics);


I don't fully understand why the source would need projected statistics

I am testing out if the issue is that the FileScanConfig is providing the wrong statistics (like maybe this line should be self.statistics rather than self.source.statistics

https://github.com/apache/datafusion/blob/1c54b38e4a4012fd8d1b4f48e2c3d6d35016bad0/datafusion/core/src/datasource/physical_plan/file_scan_config.rs#L233-L232

That's a great idea! We can't use self.source.statistics directly, because statistics match projection we're applying - so I had to apply projection before (89ed225).

This made the PR a bit messier, and I had to comment several test lines - LMK if you prefer the old version

alamb

Thank you for working on this PR @blaginin and @berkaysynnada and @mertak-synnada

I am somewhat confused about the current state of statistics as both FileSource and DataSource have statistics and it isn't clear to me how the statistics should be related. I am working to understand this more

alamb · 2025-02-27T12:39:57Z

I filed a separate ticket for the issue I was seeing in the delta-rs upgrade

Error projecting statistics in DataSourceExec #14905

I tried running the delta-rs upgrade with this change and it also fixes the delta.rs issue 🤗

I will spend more time today looking at this code / trying to help

blaginin · 2025-02-27T14:49:24Z

Thank you Andrew!! I'll resolve all the comments later today

alamb · 2025-02-27T16:23:35Z

Thank you Andrew!! I'll resolve all the comments later today

Thank you for working on this

alamb · 2025-02-28T01:32:22Z

Hi @blaginin -- I also wrote some additional tests in a different PR:

Add additional protobuf tests for plans that read parquet with projections #14924

Feel free to bring them into this PR (or we can keep them in a separate PR too)

blaginin · 2025-02-28T08:06:27Z

datafusion/core/src/datasource/file_format/parquet.rs

@@ -1934,7 +1934,8 @@ mod tests {

        // test metadata
        assert_eq!(exec.statistics()?.num_rows, Precision::Exact(8));
-        assert_eq!(exec.statistics()?.total_byte_size, Precision::Exact(671));
+        // assert_eq!(exec.statistics()?.total_byte_size, Precision::Exact(671));
+        // todo: uncomment when FileScanConfig::projection_stats puts byte size


I don't think those lines were correct, because tests had the same bug: projection was set after the source, and so source statistics (which were used here) weren't really updated properly

A proper way to fix this is set bytes size in projection_stats

Since it's a separate existing todo, I think it's better to change it in a PR on top of this one

I filed this ticket to track:

Statistics::total_byte_size does not account for projection in FileScanConfig::with_projection #14936

…-before-source

alamb

Thank you so much @blaginin @mertak-synnada and @xudong963

While I agree this using the existing total_byte_size is not correct I think we should keep the same existing behavior (to minimize downstream impacts) and handle fixing it in a follow on ticket.

I took the liberty of reverting this test change and filing an issue to discuss it separately in

Statistics::total_byte_size does not account for projection in FileScanConfig::with_projection #14936

alamb · 2025-02-28T14:40:41Z

datafusion/datasource/src/file_scan_config.rs

-            .constraints
-            .project(proj_indices)
-            .unwrap_or_else(Constraints::empty);
+        let schema = self.projected_schema();


this looks really nice now

alamb · 2025-02-28T14:41:04Z

datafusion/proto/tests/cases/roundtrip_physical_plan.rs

@@ -1626,7 +1659,6 @@ async fn roundtrip_parquet_select_star_predicate() -> Result<()> {
    roundtrip_test_sql_with_context(sql, &ctx).await
 }

-#[ignore = "Test failing due to https://github.com/apache/datafusion/issues/14679"]


alamb · 2025-02-28T14:50:52Z

datafusion/core/src/datasource/file_format/parquet.rs

@@ -1934,7 +1934,8 @@ mod tests {

        // test metadata
        assert_eq!(exec.statistics()?.num_rows, Precision::Exact(8));
-        assert_eq!(exec.statistics()?.total_byte_size, Precision::Exact(671));
+        // assert_eq!(exec.statistics()?.total_byte_size, Precision::Exact(671));
+        // todo: uncomment when FileScanConfig::projection_stats puts byte size


I filed this ticket to track:

Statistics::total_byte_size does not account for projection in FileScanConfig::with_projection #14936

alamb · 2025-02-28T15:18:57Z

It would be nice if we could avoid having Statistics on both FileScanConfig and file_source. I played around with this but hit some issues and don't have time to pursue it now. Filed this ticket to track:

Remove redundant statistics from FileScanConfig #14937

alamb · 2025-02-28T15:27:03Z

Thanks again for sticking with this @blaginin -- the result looks really nice

blaginin · 2025-02-28T15:54:04Z

Thank you for the help! 🥹 I'll do follow up prs

alamb · 2025-02-28T16:00:17Z

Thank you for the help! 🥹 I'll do follow up prs

Thank you so much

alamb

This PR appears to have accidentally updated the datafusion-testing pin - I will fix that

PR to fix:

Fix failing extended sqlitetest on main / update datafusion-testing pin #14940

Set projection before configuring the source

42d4403

github-actions bot added core Core DataFusion crate proto Related to proto crate labels Feb 15, 2025

Refresh source manually

a8709ff

blaginin added 2 commits February 19, 2025 20:57

Merge branch 'refs/heads/main' into bugfix/set-projection-before-source

582aec2

Update statistics on proj / partition columns update

1b8a39c

github-actions bot removed the proto Related to proto crate label Feb 19, 2025

blaginin marked this pull request as ready for review February 19, 2025 21:39

xudong963 self-requested a review February 23, 2025 14:42

xudong963 reviewed Feb 25, 2025

View reviewed changes

berkaysynnada approved these changes Feb 26, 2025

View reviewed changes

alamb reviewed Feb 26, 2025

View reviewed changes

alamb mentioned this pull request Feb 27, 2025

Error projecting statistics in DataSourceExec #14905

Closed

Merge main

54bdbb1

github-actions bot removed the core Core DataFusion crate label Feb 27, 2025

Use FileScanConfig own source

89ed225

github-actions bot added the proto Related to proto crate label Feb 28, 2025

blaginin added 2 commits February 28, 2025 01:03

Extend test to ensure stats are different

9dd4e38

Unify names

09a2f6e

alamb mentioned this pull request Feb 28, 2025

Add additional protobuf tests for plans that read parquet with projections #14924

Merged

alamb mentioned this pull request Feb 28, 2025

Release DataFusion 46.0.0 #14123

Open

26 tasks

Comment total_byte_size in tests

e87ffb9

github-actions bot added the core Core DataFusion crate label Feb 28, 2025

blaginin commented Feb 28, 2025

View reviewed changes

blaginin added 3 commits February 28, 2025 12:14

Use source stats as a base

616486d

Merge main & unskip the new test

1ac9bbf

Return correct stats in the ParquetSource mock

a74637c

alamb mentioned this pull request Feb 28, 2025

Statistics::total_byte_size does not account for projection in FileScanConfig::with_projection #14936

Open

alamb added 4 commits February 28, 2025 09:53

Revert test changes, add follow on ticket

dbd4f67

Merge remote-tracking branch 'apache/main' into bugfix/set-projection…

7255a16

…-before-source

Revert statistics total_byte_count change

9ca4e9d

Update test

e9ef934

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 28, 2025

alamb approved these changes Feb 28, 2025

View reviewed changes

alamb mentioned this pull request Feb 28, 2025

Minor: use FileScanConfig builder API in some tests #14938

Open

alamb merged commit 2fd558f into apache:main Feb 28, 2025
24 checks passed

alamb reviewed Feb 28, 2025

View reviewed changes

alamb mentioned this pull request Feb 28, 2025

Fix failing extended sqlitetest on main / update datafusion-testing pin #14940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set projection before configuring the source #14685

Set projection before configuring the source #14685

blaginin commented Feb 15, 2025 •

edited by alamb

Loading

mertak-synnada commented Feb 17, 2025

blaginin commented Feb 17, 2025

mertak-synnada commented Feb 19, 2025

blaginin commented Feb 19, 2025

milenkovicm commented Feb 24, 2025

xudong963 Feb 25, 2025

berkaysynnada left a comment

berkaysynnada Feb 26, 2025 •

edited

Loading

blaginin Feb 28, 2025

alamb commented Feb 26, 2025

alamb Feb 26, 2025

blaginin Feb 28, 2025 •

edited

Loading

alamb left a comment

alamb commented Feb 27, 2025

blaginin commented Feb 27, 2025

alamb commented Feb 27, 2025

alamb commented Feb 28, 2025 •

edited

Loading

blaginin Feb 28, 2025

alamb Feb 28, 2025

alamb left a comment •

edited

Loading

alamb Feb 28, 2025

alamb Feb 28, 2025

alamb Feb 28, 2025

alamb commented Feb 28, 2025

alamb commented Feb 28, 2025

blaginin commented Feb 28, 2025

alamb commented Feb 28, 2025 •

edited

Loading

alamb left a comment •

edited

Loading

Set projection before configuring the source #14685

Set projection before configuring the source #14685

Conversation

blaginin commented Feb 15, 2025 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mertak-synnada commented Feb 17, 2025

blaginin commented Feb 17, 2025

mertak-synnada commented Feb 19, 2025

blaginin commented Feb 19, 2025

milenkovicm commented Feb 24, 2025

Choose a reason for hiding this comment

berkaysynnada left a comment

Choose a reason for hiding this comment

berkaysynnada Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 26, 2025

Choose a reason for hiding this comment

blaginin Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Feb 27, 2025

blaginin commented Feb 27, 2025

alamb commented Feb 27, 2025

alamb commented Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 28, 2025

alamb commented Feb 28, 2025

blaginin commented Feb 28, 2025

alamb commented Feb 28, 2025 • edited Loading

alamb left a comment • edited Loading

Choose a reason for hiding this comment

blaginin commented Feb 15, 2025 •

edited by alamb

Loading

berkaysynnada Feb 26, 2025 •

edited

Loading

blaginin Feb 28, 2025 •

edited

Loading

alamb commented Feb 28, 2025 •

edited

Loading

alamb left a comment •

edited

Loading

alamb commented Feb 28, 2025 •

edited

Loading

alamb left a comment •

edited

Loading