[core] Support statistic with time travel #4251

xuzifu666 · 2024-09-24T15:14:19Z

Purpose

Scenario：user query statistic table by snapshot-id and analyzed table multiple times. Such as user can do the analyzed at regular time and collect the statistic metric to monitor.
Currently paimon query statistic system table with snapshot_id would error out due to PaimonAnalyzeTableColumnCommand##commitStatistics not keep the real snapshot id the same with statistic snapshot_id，the pr is aimed to fix it.
before fix:

after fix:

Linked issue: close #xxx

Tests

AnalyzeTableTestBase "Paimon analyze: test statistic system table with predicate"

API and Format

Documentation

wwj6591812 · 2024-09-25T01:01:52Z

paimon-core/src/main/java/org/apache/paimon/table/system/StatisticTable.java

+                        (Long)
+                                snapshotIdPredicate
+                                        .visit(LeafPredicateExtractor.INSTANCE)
+                                        .get("snapshot_id")


Lots of "snapshot_id" in this class, could you add:
private static final String SNAPSHOT_ID = "snapshot_id";

And TABLE_TYPE in this class should be private.

wwj6591812 · 2024-09-25T01:02:27Z

paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

+    public Optional<Statistics> statistics(Long snapshotId) {
+        if (!snapshotManager().snapshotExists(snapshotId)) {
+            throw new SnapshotNotExistException(
+                    String.format("snapshot id: %s is not exisit", snapshotId.toString()));


Change "snapshotId.toString()" to "snapshotId".

wwj6591812 · 2024-09-25T01:02:49Z

paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

+                    String.format("snapshot id: %s is not exisit", snapshotId.toString()));
+        }
+
+        Long latestSnapshotId = snapshotManager().latestSnapshotId();


latestSnapshotId may be null, you should add a check.

wwj6591812 · 2024-09-25T01:03:42Z

paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

+    public Optional<Statistics> statistics(Long snapshotId) {
+        if (!snapshotManager().snapshotExists(snapshotId)) {
+            throw new SnapshotNotExistException(
+                    String.format("snapshot id: %s is not exisit", snapshotId.toString()));


Change "is not exisit" to "is not existed";

wwj6591812 · 2024-09-25T01:07:09Z

paimon-core/src/main/java/org/apache/paimon/table/system/StatisticTable.java

-        private StatisticSplit(Path location) {
+        private final @Nullable LeafPredicate snapshotIdPredicate;
+
+        private StatisticSplit(Path location, LeafPredicate snapshotIdPredicate) {


In line 159, add @nullable before "LeafPredicate snapshotIdPredicate".

wwj6591812 · 2024-09-25T01:31:07Z

paimon-core/src/main/java/org/apache/paimon/table/system/StatisticTable.java

-    public static final RowType TABLE_TYPE =
+    private static final String SNAPSHOT_ID = "snapshot_id";
+
+    private static final RowType TABLE_TYPE =
            new RowType(
                    Arrays.asList(
                            new DataField(0, "snapshot_id", new BigIntType(false)),


SNAPSHOT_ID

wwj6591812 · 2024-09-25T01:31:28Z

paimon-core/src/main/java/org/apache/paimon/table/system/StatisticTable.java

+
+            Map<String, LeafPredicate> leafPredicates =
+                    predicate.visit(LeafPredicateExtractor.INSTANCE);
+            snapshotIdPredicate = leafPredicates.get("snapshot_id");


SNAPSHOT_ID

also in line 110

TABLE_TYPE maybe not need set with constant value，keep the style with other table seems better，other position would change to SNAPSHOT_ID.

wwj6591812 · 2024-09-25T01:32:25Z

Good Job!
But when I review your code, I found there is no doc for statistic system table.
Could you add?

xuzifu666 · 2024-09-25T01:33:28Z

Good Job! But when I review your code, I found there is no doc for statistic system table. Could you add?

Addressed, Thanks for @wwj6591812 reivew, I would add a doc for it.

wwj6591812 · 2024-09-25T06:01:46Z

+1

JingsongLi · 2024-09-26T01:46:49Z

paimon-core/src/main/java/org/apache/paimon/table/Table.java

@@ -74,6 +74,9 @@ default String fullName() {
    @Experimental
    Optional<Statistics> statistics();

+    @Experimental
+    Optional<Statistics> statistics(Long snapshotId);


Should be a long instead of Long

JingsongLi · 2024-09-26T01:47:35Z

paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

        Snapshot latestSnapshot = snapshotManager().latestSnapshot();
        if (latestSnapshot != null) {
            return store().newStatsFileHandler().readStats(latestSnapshot);
        }
        return Optional.empty();
    }

+    @Override
+    public Optional<Statistics> statistics(Long snapshotId) {
+        if (!snapshotManager().snapshotExists(snapshotId)) {


Just return store().newStatsFileHandler().readStats(latestSnapshot);?

Yes, it should return store().newStatsFileHandler().readStats(latestSnapshot) better. Thanks~ @JingsongLi

JingsongLi · 2024-09-26T01:48:49Z

paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

@@ -166,14 +166,43 @@ public Identifier identifier() {

    @Override
    public Optional<Statistics> statistics() {
-        // todo: support time travel
        Snapshot latestSnapshot = snapshotManager().latestSnapshot();


Maybe just here to respect time travel options.

JingsongLi · 2024-09-26T15:56:34Z

paimon-core/src/main/java/org/apache/paimon/table/Table.java

@@ -74,6 +74,9 @@ default String fullName() {
    @Experimental
    Optional<Statistics> statistics();

+    @Experimental
+    Optional<Statistics> statistics(long snapshotId);


Can we avoid introducing this method?

Maybe we can Table.copy('scan.snapshot-id', 'xxx').statistics to get stats for snapshot.

Nice reminder，from this way can implement time travel for statistic. Had change it.

JingsongLi · 2024-09-26T15:58:12Z

paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java

+                    return statistics;
+                }
+            }
+            latestSnapshotId--;


We don't need to find the snapshot with ANALYZE commit. The snapshot will inherit its parent snapshot.

So I said, just return the stats of this snapshot.

According the logic maybe we need traversal. Because StatsFileHandler##readStats from a snapshotid，can only get the latest analyzed snapshot，but from the logic statistic file snapshot_id may less than the anaylzed snapshot about it. I added a case to the ut at end which may refer it.

xuzifu666 · 2024-09-27T06:42:50Z

traversal logic do a optimize, before： need from latest snapshot to the target analyzed snapshot, after： change from the nearest snapshot to the target analyzed snapshot. @JingsongLi

xuzifu666 · 2024-09-27T08:49:01Z

After discuss， change to query by real snapshot id. @JingsongLi

JingsongLi

+1

xuyu added 5 commits September 24, 2024 22:42

[core] Support statistic system table query with snapshot_id

b6b0cc9

add more ut

67bc406

change ut

7804136

rec

cf676e2

add more case

48fe472

xuzifu666 closed this Sep 24, 2024

xuzifu666 reopened this Sep 24, 2024

wwj6591812 reviewed Sep 25, 2024

View reviewed changes

address

d9d2e10

wwj6591812 reviewed Sep 25, 2024

View reviewed changes

xuyu added 3 commits September 25, 2024 09:35

address 1

d452c74

add doc

548858b

reduce unnessary loop

e62988e

JingsongLi reviewed Sep 26, 2024

View reviewed changes

address comments

bbdbb2f

JingsongLi reviewed Sep 26, 2024

View reviewed changes

xuyu added 2 commits September 27, 2024 11:41

support time travel for statistic

516f559

fix import

77fddbe

xuzifu666 changed the title ~~[core] Support statistic system table query with snapshot_id~~ [core] Support statistic with time travel Sep 27, 2024

xuyu added 2 commits September 27, 2024 12:09

add case

fdb26ba

optimize query

9100a8f

xuyu added 2 commits September 27, 2024 16:24

query change

cab9d1c

fix

c6e74f7

fix ut

82fba79

JingsongLi approved these changes Oct 8, 2024

View reviewed changes

JingsongLi merged commit 7a08c89 into apache:master Oct 8, 2024
11 checks passed

xuzifu666 mentioned this pull request Oct 8, 2024

[core] Manifest and Statistic support query by tag name #4291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Support statistic with time travel #4251

[core] Support statistic with time travel #4251

xuzifu666 commented Sep 24, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024

wwj6591812 Sep 25, 2024

wwj6591812 Sep 25, 2024

wwj6591812 Sep 25, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024

wwj6591812 Sep 25, 2024

wwj6591812 Sep 25, 2024

xuzifu666 Sep 25, 2024

wwj6591812 commented Sep 25, 2024

xuzifu666 commented Sep 25, 2024

wwj6591812 commented Sep 25, 2024

JingsongLi Sep 26, 2024

xuzifu666 Sep 26, 2024

JingsongLi Sep 26, 2024

xuzifu666 Sep 26, 2024

JingsongLi Sep 26, 2024

xuzifu666 Sep 26, 2024

JingsongLi Sep 26, 2024

xuzifu666 Sep 27, 2024

JingsongLi Sep 26, 2024

xuzifu666 Sep 27, 2024

xuzifu666 Sep 27, 2024 •

edited

Loading

xuzifu666 commented Sep 27, 2024 •

edited

Loading

xuzifu666 commented Sep 27, 2024

JingsongLi left a comment

[core] Support statistic with time travel #4251

[core] Support statistic with time travel #4251

Conversation

xuzifu666 commented Sep 24, 2024 • edited Loading

Purpose

Tests

API and Format

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwj6591812 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

wwj6591812 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

wwj6591812 Sep 25, 2024 • 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

Choose a reason for hiding this comment

wwj6591812 commented Sep 25, 2024

xuzifu666 commented Sep 25, 2024

wwj6591812 commented Sep 25, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzifu666 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

xuzifu666 commented Sep 27, 2024 • edited Loading

xuzifu666 commented Sep 27, 2024

JingsongLi left a comment

Choose a reason for hiding this comment

xuzifu666 commented Sep 24, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024 •

edited

Loading

wwj6591812 Sep 25, 2024 •

edited

Loading

xuzifu666 Sep 27, 2024 •

edited

Loading

xuzifu666 commented Sep 27, 2024 •

edited

Loading