Add projectionHint to SG Parquet mode #7206
Conversation
pkg/storage/tsdb/config.go
Outdated
| f.Float64Var(&cfg.TokenBucketBytesLimiter.TouchedChunksTokenFactor, "blocks-storage.bucket-store.token-bucket-bytes-limiter.touched-chunks-token-factor", 1, "Multiplication factor used for touched chunks token") | ||
| f.IntVar(&cfg.MatchersCacheMaxItems, "blocks-storage.bucket-store.matchers-cache-max-items", 0, "Maximum number of entries in the regex matchers cache. 0 to disable.") | ||
| cfg.ParquetShardCache.RegisterFlagsWithPrefix("blocks-storage.bucket-store.", f) | ||
| f.BoolVar(&cfg.HonorProjectionHints, "blocks-storage.bucket-store.honor-projection-hints", false, "[Experimental] If enabled, Store Gateway will honor projection hints and only materialize requested labels. It is only effect when `-blocks-storage.bucket-store.bucket-store-type` is a parquet.") |
There was a problem hiding this comment.
Just to double check, if I want to actually use parquet store gateway projection, I need to also honor projection hints in Querier (but not enable parquet querier)?
There was a problem hiding this comment.
yeah, even if the Parquet Querier is disabled (using the standard Querier), the Parquet Store Gateway relies on these hints to perform projection pushdown (Querier send the projection hints).
There was a problem hiding this comment.
Seems that even if we don't enable honor projection hints in querier, it still propagates projection hints to store gateway. I don't see you enable that in the integration test.
| storageHints.ProjectionLabels = queryHints.ProjectionLabels | ||
|
|
||
| // Reset projection hints if not all parquet shard have the hash column. | ||
| if !allParquetBlocksHaveHashColumn(shards) { |
There was a problem hiding this comment.
We should only project if it is projection include. Add that check first can avoid checking all blocks of hash column when unnecessary
integration/querier_test.go
Outdated
| vec := aggResult.(model.Vector) | ||
| require.Len(t, vec, 1) | ||
| require.Equal(t, model.LabelValue("series_1"), vec[0].Metric["series_1"]) | ||
| require.Equal(t, expectedVector1[0].Value, vec[0].Value) |
There was a problem hiding this comment.
I want to avoid adding more test cases to this test.
I think overall the test is very flaky and it keeps failing for different reasons. I would prefer separate tests for parquet store gateway mode tbh.
| storageHints.ProjectionInclude = false | ||
| storageHints.ProjectionLabels = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I would also reset the projection hints if it is not projection include. Same as what we do in querier.
Line 455 in 28ad1ac
If we don't do this then it can do wrong below as we only add series hash if it is projection include. In general it is hard to estimate the cost of projection not include and it can be more expensive
Signed-off-by: SungJin1212 <[email protected]>
SungJin1212
force-pushed
the
Add-ProjectionHints-to-SG
branch
from
903d00e to
d9638cc
Compare
Signed-off-by: SungJin1212 <[email protected]>
|
@yeya24 |
2 participants
This PR adds
-blocks-storage.bucket-store.honor-projection-hintsCLI flag. If enabled, Store Gateway in Parquet mode will honor projection hints and only materialize requested labels.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]