[feat] Support cache ListFiles result cache in session level #7620

Ted-Jiang · 2023-09-22T10:06:47Z

Which issue does this PR close?

Closes #7618.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Ted-Jiang · 2023-09-22T10:08:45Z

datafusion/core/tests/parquet/file_statistics.rs

    opt: &ListingOptions,
 ) -> ListingTable {
-    let schema = opt.infer_schema(state1, table_path).await.unwrap();
+    let schema = opt
+        .infer_schema(&SessionState::default(), table_path)


Here infer_schema will call list_files as well, so create a new Session state here

Ted-Jiang · 2023-09-23T02:12:59Z

datafusion/execution/src/cache/cache_unit.rs

@@ -94,9 +94,69 @@ impl CacheAccessor<Path, Arc<Statistics>> for DefaultFileStatisticsCache {
    }
 }

+/// Collected files metadata for listing files.
+/// Cache will not invalided until user call remove or clear.


I think here keep simple, user can use their own notify system to decide call remove or clear

alamb

This looks good to me @Ted-Jiang -- thank you. The only thing I think needs to be addressed prior to merge is the addition of SessionState::default

alamb · 2023-09-23T10:35:29Z

datafusion/core/src/execution/context.rs

@@ -1405,6 +1405,15 @@ pub fn default_session_builder(config: SessionConfig) -> SessionState {
    SessionState::with_config_rt(config, Arc::new(RuntimeEnv::default()))
 }

+impl Default for SessionState {


I am worried that providing a Default for SessionState might allow subtle bugs, specifically accidentally running queries or other operations without properly threading through the configuration or runtime. I believe that is why there is no SessionState::new() or Default

🤔 this context would be nice to add as a comment to SessonState

Yes, it there are many options to set in SessionState , i just want to write less code when construct SessionState see there are already default in SessionConfig and RuntimeEnv 😂 ， it make sense let user point what conf they want.

I think we should remove the Default impl and instead have a function for tests (test_session_state() or similar) to avoid misuse of this API.

Here is a PR to add some comments about the rationale for not having a Default impl: #7654

datafusion/execution/src/cache/cache_unit.rs

datafusion/execution/src/cache/cache_manager.rs

alamb · 2023-09-23T10:41:52Z

I was thinking a little about the cache APIs here -- at the moment these are all related to ListingTable but are attached to the SessionState. I wonder if we should make a containing structure like ListingTableCache or something to contain them 🤔 .

This is not a suggested change for this PR, just something I was thinking about.

Ted-Jiang · 2023-09-24T06:17:03Z

I was thinking a little about the cache APIs here -- at the moment these are all related to ListingTable but are attached to the SessionState. I wonder if we should make a containing structure like ListingTableCache or something to contain them 🤔 .

This is not a suggested change for this PR, just something I was thinking about.

Thanks for your kindly review @alamb ! Yes the cache method here are all in ListingTable, I will check refine it later.

As this is cache , it need share the cache result in some place, i think there is only SessionState has the state can share in some level like(session), is there any other way to store the result ?

btw i see other system use global value to store global cache, is there a way datafusion use this method

alamb · 2023-09-25T11:34:55Z

btw i see other system use global value to store global cache, is there a way datafusion use this method

The way IOx does this is to setup a RuntimeConfig with a global MemoryManager and DiskManager and then create SessionContext's from that RuntimeConfig.

I think you can do something similar with the caches in this PR

suremarc · 2023-09-25T16:48:50Z

Are there any advantages to caching file listing in DataFusion instead of inside the ObjectStore? It seems to me it should be possible to implement this without any changes to DataFusion itself. There may be tradeoffs there I'm not aware of though

alamb

Thanks @Ted-Jiang -- I think we should remove the Default impl for SessionState from this PR

alamb · 2023-09-25T19:00:02Z

datafusion/core/src/execution/context.rs

@@ -1405,6 +1405,15 @@ pub fn default_session_builder(config: SessionConfig) -> SessionState {
    SessionState::with_config_rt(config, Arc::new(RuntimeEnv::default()))
 }

+impl Default for SessionState {


I think we should remove the Default impl and instead have a function for tests (test_session_state() or similar) to avoid misuse of this API.

Here is a PR to add some comments about the rationale for not having a Default impl: #7654

alamb · 2023-09-25T19:04:35Z

Are there any advantages to caching file listing in DataFusion instead of inside the ObjectStore? It seems to me it should be possible to implement this without any changes to DataFusion itself. There may be tradeoffs there I'm not aware of though

This is an excellent point @suremarc -- what do you think @Ted-Jiang of making a Caching ObjectStore implementation? As @suremarc points out, there is nothing DataFusion specific about this feature, so perhaps it would be better modeled at the ObjectStore level 🤔

alamb · 2023-09-28T16:56:52Z

Converting to draft as it is not waiting on a review -- we are now working through comments

Ted-Jiang · 2023-10-03T02:49:02Z

datafusion/core/src/test/object_store.rs

-    Arc::new(memory)
+    (
+        Arc::new(memory),
+        SessionState::with_config_rt(


Remove the default of SeesionState

Ted-Jiang · 2023-10-03T03:00:51Z

@alamb @suremarc Sorry for the delay.

making a Caching ObjectStore implementation?

As i have check the some remote store client like hdfs there are no cache result in client side, IMO doing this is ObjectStore may be difficult define the work scope (ensure correctness), in datafusion we can support in in session level to not effect others 🤔

Ted-Jiang · 2023-10-06T12:14:31Z

@alamb could you take a look plz

suremarc · 2023-10-06T15:57:56Z

@alamb @suremarc Sorry for the delay.

making a Caching ObjectStore implementation?

As i have check the some remote store client like hdfs there are no cache result in client side, IMO doing this is ObjectStore may be difficult define the work scope (ensure correctness), in datafusion we can support in in session level to not effect others 🤔

I agree it would be more difficult, having implemented some caching of my own recently. A full implementation would require dealing with invalidation/consistency, particularly for update operations, as well as supporting nontrivial APIs like list_with_delimiter. It would be nice if we could do it there but it's unclear to me if it can be done perfectly.

On that note, I'd like to point out that this cache implementation won't avoid hitting object storage if your table has partition columns, because in that case DataFusion will instead call list_with_delimiter recursively, starting from the top-level path. It would still be an improvement, but I am not sure if this will speed up query planning as much as you want. Just wanted to make sure you're aware of this, as I previously had thought that simply caching list_all_files would avoid hitting object storage altogether.

alamb

Thanks @Ted-Jiang and @suremarc

Do you have any performance measurements you can shar @Ted-Jiang about how much this feature increases performance for your usecase?

datafusion/execution/src/cache/cache_manager.rs

Co-authored-by: Andrew Lamb <[email protected]>

Ted-Jiang · 2023-10-07T03:15:23Z

Thanks @Ted-Jiang and @suremarc

Do you have any performance measurements you can shar @Ted-Jiang about how much this feature increases performance for your usecase?

We try to fix the situation when calling remote storage list file statistics sometimes are not stable:
we run one query avg cost 4 seconds, but sometimes it cost double time, we print some log for debug:

2023-09-12T06:10:59.098033Z  INFO tokio-runtime-worker ThreadId(16) datafusion::execution::context: Datafusion optimize logical plan cost: 10.852243ms
2023-09-12T06:11:03.309479Z  INFO tokio-runtime-worker ThreadId(16) datafusion::execution::context: Datafusion create physical plan cost: **_4.2222994s_**
2023-09-12T06:11:03.309543Z  INFO tokio-runtime-worker ThreadId(16) ballista_scheduler::planner: planning query stages for job VyQVhat
2023-09-12T06:11:06.439247Z  INFO tokio-runtime-worker ThreadId(20) ballista_scheduler::display: === [VyQVhat/1] Stage finished, physical plan with metrics ===

we saw half time cost in create physical plan 🤣 (btw we pass logic plan LIST_TABLE to datafusion in out front end (written in JAVA))

after enable cache we fix this problem the query time keep stable.

btw we have fix the consistency in our java side, because out sys build kind of materialized view, if we we change the source data the path as the cache key also change.

I'd like to point out that this cache implementation won't avoid hitting object storage if your table has partition columns, because in that case DataFusion will instead call list_with_delimiter recursively

@suremarc thanks for point this out 👍 i will figure out this later.

alamb · 2023-10-07T10:23:55Z

Thanks again @Ted-Jiang and @suremarc

Ted-Jiang added 2 commits September 22, 2023 18:01

support cache list files under path in session

8428ebc

add test

1307075

github-actions bot added the core Core DataFusion crate label Sep 22, 2023

Ted-Jiang commented Sep 22, 2023

View reviewed changes

Ted-Jiang added 2 commits September 23, 2023 08:32

fix clippy

734ab2d

fix clippy2

76ee890

Ted-Jiang commented Sep 23, 2023

View reviewed changes

Ted-Jiang requested a review from alamb September 23, 2023 02:13

alamb reviewed Sep 23, 2023

View reviewed changes

Ted-Jiang added 2 commits September 24, 2023 13:44

Merge remote-tracking branch 'up/main' into 7618

cfc44b9

fix comment

5e2ba2f

Ted-Jiang requested a review from alamb September 24, 2023 06:19

alamb reviewed Sep 25, 2023

View reviewed changes

alamb marked this pull request as draft September 28, 2023 16:56

Merge remote-tracking branch 'up/main' into 7618

c807225

Ted-Jiang force-pushed the 7618 branch from 1dd59c8 to c807225 Compare October 3, 2023 02:46

Ted-Jiang commented Oct 3, 2023

View reviewed changes

Ted-Jiang marked this pull request as ready for review October 3, 2023 04:20

Ted-Jiang requested a review from alamb October 3, 2023 10:53

alamb approved these changes Oct 6, 2023

View reviewed changes

datafusion/execution/src/cache/cache_manager.rs Outdated Show resolved Hide resolved

Update datafusion/execution/src/cache/cache_manager.rs

a29f172

Co-authored-by: Andrew Lamb <[email protected]>

fix fmt

8915369

Ted-Jiang force-pushed the 7618 branch from 4eb7bbe to 8915369 Compare October 7, 2023 03:02

Ted-Jiang force-pushed the 7618 branch from 8915369 to bf1bb88 Compare October 7, 2023 03:29

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) substrait labels Oct 7, 2023

Ted-Jiang force-pushed the 7618 branch from bf1bb88 to 0db9945 Compare October 7, 2023 03:31

fix api change

48232ec

Ted-Jiang force-pushed the 7618 branch from 0db9945 to 48232ec Compare October 7, 2023 03:48

github-actions bot removed sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) substrait labels Oct 7, 2023

fix api change2

b7f0ccc

alamb merged commit 093b775 into apache:main Oct 7, 2023
22 checks passed

matthewgapp mentioned this pull request Jan 11, 2024

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

Closed

henrifroese mentioned this pull request Mar 17, 2024

Partitioned object store lists all files on every query when using hive-partitioned parquet files #9654

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Support cache ListFiles result cache in session level #7620

[feat] Support cache ListFiles result cache in session level #7620

Ted-Jiang commented Sep 22, 2023

Ted-Jiang Sep 22, 2023

Ted-Jiang Sep 23, 2023

alamb left a comment

alamb Sep 23, 2023

Ted-Jiang Sep 24, 2023

alamb Sep 25, 2023

alamb commented Sep 23, 2023

Ted-Jiang commented Sep 24, 2023 •

edited

Loading

alamb commented Sep 25, 2023

suremarc commented Sep 25, 2023

alamb left a comment •

edited

Loading

alamb Sep 25, 2023

alamb commented Sep 25, 2023

alamb commented Sep 28, 2023

Ted-Jiang Oct 3, 2023

Ted-Jiang commented Oct 3, 2023 •

edited

Loading

Ted-Jiang commented Oct 6, 2023

suremarc commented Oct 6, 2023

alamb left a comment

Ted-Jiang commented Oct 7, 2023 •

edited

Loading

alamb commented Oct 7, 2023

[feat] Support cache ListFiles result cache in session level #7620

[feat] Support cache ListFiles result cache in session level #7620

Conversation

Ted-Jiang commented Sep 22, 2023

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?

Ted-Jiang Sep 22, 2023

Choose a reason for hiding this comment

Ted-Jiang Sep 23, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 23, 2023

Choose a reason for hiding this comment

Ted-Jiang Sep 24, 2023

Choose a reason for hiding this comment

alamb Sep 25, 2023

Choose a reason for hiding this comment

alamb commented Sep 23, 2023

Ted-Jiang commented Sep 24, 2023 • edited Loading

alamb commented Sep 25, 2023

suremarc commented Sep 25, 2023

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb Sep 25, 2023

Choose a reason for hiding this comment

alamb commented Sep 25, 2023

alamb commented Sep 28, 2023

Ted-Jiang Oct 3, 2023

Choose a reason for hiding this comment

Ted-Jiang commented Oct 3, 2023 • edited Loading

Ted-Jiang commented Oct 6, 2023

suremarc commented Oct 6, 2023

alamb left a comment

Choose a reason for hiding this comment

Ted-Jiang commented Oct 7, 2023 • edited Loading

alamb commented Oct 7, 2023

Ted-Jiang commented Sep 24, 2023 •

edited

Loading

alamb left a comment •

edited

Loading

Ted-Jiang commented Oct 3, 2023 •

edited

Loading

Ted-Jiang commented Oct 7, 2023 •

edited

Loading