This is an automated comment for commit 1bae2d9 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Right now, these globs work as described in #49929:
Imagine we have some <bucket>, and a lot of files that can be described as path{1,2,3.......100000}/file*.csv. From these files, we only want to select path{1,2,3}/file*.csv. When we do e.g. SELECT * FROM s3('<bucket>/path{1,2,3}/file*.csv'), CH retrieves all the files that have a prefix <bucket>/path. This can be thousands of files we don't need. This makes some simple queries impossible to run.

There are two solutions I see:

1. Doing on-demand (lazy) calls to ListObjectsV2 for each option inside {} glob, in our case it will be listing keys with prefix path1/file, then with prefix path2/file, then with prefix path3/file. This significantly reduces number of calls to ListObjectsV2. But when initializing StorageS3, it only performs one call, all other calls are performed only when CH needs to read the next file. This leads to a problem:
   This breaks a test, and I am not sure if it really breaks real workflows. The problem is with test test_schema_inference_cache with globs. To test it, currently we run a describe query (e.g.DESC s3(bucket/file{1,2,3,4}.csvvvv), then check for cache misses/hits on next queries. This lazy approach will not work for the test: describe query will not read all possible files from S3, it will take those that have already been read on initialization, and also put them into schema cache.
   This works now because the test is trivial: there are only 4 files, that are also read on Storage initialization (current behavior: read all keys with prefix file), and the cache is populated properly.
   With this new approach, in this test only file1.csvvvv will be put into cache. BUT select query will work perfect, and all files will be added to schema cache.
   Actually, it is very close to the current behavior. If each selection has many underlying files, this difference will be unnoticeable.

2. The second approach is similar to the first, but we do all these calls on initialization and store a complete list of files to be read. This can be rather expensive, as there can be thousands of files.

I like the first option more. I am not sure if people really use describe queries to populate the cache.

@kssenii , please take a look, this time tests are OK, and I see no bugs left :)

@kssenii ping

On the latest changes (c9a3159 and 686ea6a):

S3 iterator is lazy: it looks for new files only when necessary, and each ListObjectsV2 lists 1000 objects. First ListObjectsV2 is done on initialization of the iterator itself.
And here is the thing: the estimation of number of keys was solely based on number of suitable files found in this first portion of listed objects. If we have a very "general" glob or if we have a strong filter, it is a common case that none of the objects satisfy the criteria, and reading from S3 throws to "failsafe" mode, reading objects one by one.

A couple of examples:

• generic_prefix**.someformat with WHERE filtering by _path and there are no keys matching the filter among the first batch of files listed;
• prefix/{a,b,c}*.someformat -- similar to the above, but after changes in this PR it would become a more common thing. Imagine there are no keys matching prefix/a*.someformat (which is listed first, on iterator initialization). Then, CH would read all these files sequentially.

A reproducible example of good vs bad performance con be found in this gist

TL;DR: IMO it is better to make reading from S3 "more parallel" in situations when we don't have any idea about how many objects we will read.

Test fails

• Integration tests (tsan) [1/6]:
  • test_mutation_with_broken_projection -- Flaky tests test_broken_projections #63002
• Stateless tests (tsan, s3 storage) [4/5]
  • 03000_traverse_shadow_system_data_paths -- Flaky test 03000_traverse_shadow_system_data_paths #63003
• Stateless tests (tsan) [1/5]
  • 03094_grouparraysorted_memory -- Flaky test 03094_grouparraysorted_memory #63086
• Stress test (asan) -- it goes somewhere to DB::SerializationArray (according to the stacktrace), looks unrelated to this PR.

@kssenii could you please take a look?

@kssenii I will make some changes (simplify things) like stated here: #62120 (comment)
Could you also take a look after it?

UPD: Stress test (ubsan) is unrelated. It is a coincidence, I see similar one in master: https://s3.amazonaws.com/clickhouse-test-reports/0/21cbbdd983b092945a4fb6015179971d18c4dab9/stress_test__debug_.html

The Stress test (ubsan) fail looks strange.

Its report says that a query SELECT count() FROM s3(\'http://localhost:11111/test/test_1/test_INT_MAX.tsv\'); was hung. But this query is not related to the changes in this PR. This URI has no globs, DisclosedGlobIterator is not used, so none of the modified code could possibly be run.

Maybe it's just a coincidence, none of the previous commits had it.

Fails (not related):

• Stress test (ubsan) -- Improve S3 glob performance #62120 (comment)
• Stateless tests (debug, s3 storage) [2/6]:
  • 02362_part_log_merge_algorithm -- Flaky test 02362_part_log_merge_algorithm #63491
• Stateless tests (debug, s3 storage) [6/6]:
  • 02680_mysql_ast_logical_err -- Flaky test 02680_mysql_ast_logical_err #63492