Add new configuration item `listing_table_ignore_subdirectory` by Asura7969 · Pull Request #8565 · apache/datafusion

Asura7969 · 2023-12-16T14:22:04Z

Which issue does this PR close?

Closes #8524.

Rationale for this change

Consistent behavior with duckdb and hive

What changes are included in this PR?

add config listing_table_ignore_subdirectory
When scanning file paths, whether to ignore subdirectory files, ignored by default (true)

NOTE: When reading a partitioned table, listing_table_ignore_subdirectory is always equal to false, even if set to true

Are these changes tested?

add sqllogictest

Are there any user-facing changes?

ListingTableUrl::contains method add parameters

Asura7969 · 2023-12-16T16:00:41Z

            .read_parquet(
                // it was reported that when a path contains // (two consecutive separator) no files were found
                // in this test, regardless of parquet_test_data() value, our path now contains a //
-                format!("{}/..//*/alltypes_plain*.parquet", parquet_test_data()),


Should we add regex matching to directories?🤔

I don't fully understand this question

alamb

Thank you @Asura7969 -- this code looks good to me, but I think we need some test coverage (I am not sure the test in this PR covers the changes)

Could you maybe create a sqllogictest test for this? So like write three files:

Like this

table/1.parquet
table/subdir/2.parquet
table/3.parquet

And then verify that with ignore_subdirectory to true (default) only the rows from 1.parquet and 3.parquet appear in the output but when the setting is set to false then the rows from all three files are present?

There is an example of creating and reading parquet files in a sqllogictest in these two files:

https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/parquet.slt (👋 @hiltontj -- we are already using it!)

https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/repartition_scan.slt

alamb · 2023-12-19T19:08:16Z

            .read_parquet(
                // it was reported that when a path contains // (two consecutive separator) no files were found
                // in this test, regardless of parquet_test_data() value, our path now contains a //
-                format!("{}/..//*/alltypes_plain*.parquet", parquet_test_data()),


I don't fully understand this question

alamb · 2023-12-19T19:09:26Z

        assert_eq!(a, b);
        assert!(a.prefix.as_ref().ends_with("bar/baz"));
+
+        let url = ListingTableUrl::parse("../foo/*.parquet").unwrap();


I am probably missing something here, but how does this test the new code? I don't see it passing in ignore_subdirectory

Yes, it's really not obvious here (actually in ListingTableUrl.contains), I would create a sqllogictest as you suggested

alamb · 2023-12-19T19:12:21Z

 datafusion.execution.batch_size 8192
 datafusion.execution.coalesce_batches true
 datafusion.execution.collect_statistics false
+datafusion.execution.ignore_subdirectory true


Could we use a name that gives some context about when the ignore_subdirectory is actually used?

For example, maybe like listing_table_ignore_subdirectory. Or maybe even it is time to create a whole new category of configuration for listing tables datafusion.listing_table.ignore_subdirectory 🤔

I used listing_table_ignore_subdirectory

alamb · 2023-12-20T22:38:51Z

+--------CsvExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/core/tests/data/partitioned_table/c_date=2018-11-13/timestamps.csv], [WORKSPACE_ROOT/datafusion/core/tests/data/partitioned_table/c_date=2018-12-13/timestamps.csv]]}, has_header=false
+
+query I
+select count(*) from partition_csv_table;


I don't understand what this test is testing -- in both cases the table has 4 rows (aka there is no data in a subdirectory to ignore, right)?

When reading a partitioned table, listing_table_ignore_subdirectory is always equal to false, even if set to true，But this test seems a bit redundant, i will clean up

alamb · 2023-12-20T22:41:21Z

+12
+
+query I
+select count(*) from t2_with_subdirectory;


It is cool to see the different rows but I don't understand the need for all the different tables and explain plans

I think we can get coverage by simply creating the equivalent of t2_with_subdirectory and showing that it returns 12 rows when

set datafusion.execution.listing_table_ignore_subdirectory = false;

And 9 when

set datafusion.execution.listing_table_ignore_subdirectory = true;

Thank you for your suggestion, I understand

# Conflicts: # datafusion/sqllogictest/test_files/parquet.slt

Asura7969 · 2023-12-21T01:55:20Z

+        /// When scanning file paths, whether to ignore subdirectory files,
+        /// ignored by default (true), when reading a partitioned table,
+        /// `listing_table_ignore_subdirectory` is always equal to false, even if set to true


I updated the description information, do you agree? @alamb

I don't understand what this is trying to say 🤔

## read partition table CREATE EXTERNAL TABLE csv_with_timestamps ( name VARCHAR, ts TIMESTAMP, c_date DATE, ) STORED AS CSV PARTITIONED BY (c_date) LOCATION '../core/tests/data/partitioned_table'; set datafusion.execution.listing_table_ignore_subdirectory = true; select count(*) from partition_tbale; ## return 4

Ah, got it -- thank you -- I will propose a clarification in a follow on PR

alamb

Thank you @Asura7969 -- this looks great

alamb · 2023-12-21T20:32:26Z

 0 2014-08-27T14:00:00Z Timestamp(Millisecond, Some("UTC"))
 0 2014-08-27T14:00:00Z Timestamp(Millisecond, Some("UTC"))

+# Test config listing_table_ignore_subdirectory:


Asura7969 added 6 commits December 14, 2023 17:35

init

df20098

test

424d665

add config

b2d288a

Merge branch 'main' into fix_list_all_files

945bf17

rename

bbeffd7

doc

abb7650

github-actions Bot added the core Core DataFusion crate label Dec 16, 2023

fix doc

e69183b

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Dec 16, 2023

Asura7969 commented Dec 16, 2023

View reviewed changes

This was referenced Dec 17, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 11, 2023 #8490

Closed

DataFusion weekly project plan (Andrew Lamb) - Dec 18, 2023 #8577

Closed

alamb reviewed Dec 19, 2023

View reviewed changes

Asura7969 added 5 commits December 20, 2023 08:58

Merge branch 'main' into fix_list_all_files

36e0c8d

add sqllogictests & rename

aff207f

fmt & fix test

7a7c8f8

clippy

e745b91

test read partition table

3ee2e61

alamb reviewed Dec 20, 2023

View reviewed changes

Asura7969 added 2 commits December 21, 2023 09:46

simplify testing

9ea237c

Merge branch 'main' into fix_list_all_files

5632324

# Conflicts: # datafusion/sqllogictest/test_files/parquet.slt

Asura7969 commented Dec 21, 2023

View reviewed changes

simplify testing

fe52098

alamb approved these changes Dec 21, 2023

View reviewed changes

Asura7969 changed the title ~~Add new configuration item ignore_subdirectory~~ Add new configuration item listing_table_ignore_subdirectory Dec 22, 2023

alamb merged commit ba46434 into apache:main Dec 22, 2023

alamb mentioned this pull request Dec 22, 2023

Minor: improve listing_table_ignore_subdirectory config documentation #8634

Merged

Asura7969 deleted the fix_list_all_files branch December 23, 2023 02:58

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Uh oh!

Conversation

Asura7969 commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Asura7969 Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Asura7969 commented Dec 16, 2023 •

edited

Loading

Asura7969 Dec 21, 2023 •

edited

Loading