Skip to content

Measure split listing time per table per query.#14713

Closed
Dith3r wants to merge 1 commit intotrinodb:masterfrom
Dith3r:issues/13921
Closed

Measure split listing time per table per query.#14713
Dith3r wants to merge 1 commit intotrinodb:masterfrom
Dith3r:issues/13921

Conversation

@Dith3r
Copy link
Copy Markdown
Member

@Dith3r Dith3r commented Oct 21, 2022

Description

Address issue #13921. Adds split distribution per table and per query.

Example output:

"stageStats": {
        "schedulingComplete": "2022-10-27T11:25:04.939Z",
        "getSplitDistribution": {
            "count": 1.0,
            "total": 6792.0,
            "p01": 6792.0,
            "p05": 6792.0,
            "p10": 6792.0,
            "p25": 6792.0,
            "p50": 6792.0,
            "p75": 6792.0,
            "p90": 6792.0,
            "p95": 6792.0,
            "p99": 6792.0,
            "min": 6792.0,
            "max": 6792.0,
            "avg": 6792.0
        },
        "tableGetSplitDistribution": [
            {
                "planNodeId": "0",
                "splitDistribution": {
                    "count": 1.0,
                    "total": 6792.0,
                    "p01": 6792.0,
                    "p05": 6792.0,
                    "p10": 6792.0,
                    "p25": 6792.0,
                    "p50": 6792.0,
                    "p75": 6792.0,
                    "p90": 6792.0,
                    "p95": 6792.0,
                    "p99": 6792.0,
                    "min": 6792.0,
                    "max": 6792.0,
                    "avg": 6792.0
                }
            }
        ]
    }

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Oct 21, 2022
@Dith3r Dith3r requested a review from sopel39 October 21, 2022 13:59
@Dith3r Dith3r force-pushed the issues/13921 branch 2 times, most recently from 1e676f1 to 3c30f98 Compare October 21, 2022 14:51
Copy link
Copy Markdown
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check manually that it reports stats correctly (please paste an example)

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Oct 21, 2022

The next step is to expose is via io.trino.spi.eventlistener.QueryStatistics (can be separate PR)

@Dith3r Dith3r force-pushed the issues/13921 branch 2 times, most recently from ed15192 to 32a6f41 Compare October 21, 2022 15:08
@findepi
Copy link
Copy Markdown
Member

findepi commented Oct 26, 2022

Address issue #13921. Adds split distribution per table and per query.

How will it be available?

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Oct 27, 2022

How will it be available?

@findepi Event listener

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented Oct 27, 2022

@Dith3r is this ready for review?

@Dith3r
Copy link
Copy Markdown
Member Author

Dith3r commented Oct 27, 2022

After merging #14785 I will update this PR. There are few changes.

@Dith3r Dith3r marked this pull request as ready for review October 28, 2022 06:25
@Dith3r Dith3r requested a review from sopel39 October 28, 2022 08:49
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a separate commit/PR, we can also update PlanPrinter to print this information for table scans in EXPLAIN ANALYZE VERBOSE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what do we gain by knowing the distribution of get-splits time per table scan instead of the simpler cumulative time taken for get-splits per table scan ?
One file listing operation might feed multiple batches of splits, so it's possible that the distribution will just capture a few high values and many low values.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With distribution, we have information about total time needed for getting splits, count, and information if there are some outliers splits.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that some get-split batches having an outlier time taken could be normal and then it wouldn't be clear what insight we might get from having a distribution.
Could you check what kind of results we get on an unpartitioned table with large number of files and a partitioned table with lots of partitions ?


long start = System.nanoTime();
addSuccessCallback(nextSplitBatchFuture, () -> stageExecution.recordGetSplitTime(start));
addSuccessCallback(nextSplitBatchFuture, () -> stageExecution.recordGetSplitTime(partitionedNode, start));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hive connector, splits are loaded by the connector in a background thread (See BackgroundHiveSplitLoader), so recording time taken here will probably miss the actual work done in listing files by the hive connector.
@sopel39 maybe we need to extend ConnectorSplitSource to allow connector to provide metrics about split generation.

@Dith3r Dith3r closed this Nov 2, 2022
@Dith3r Dith3r deleted the issues/13921 branch November 2, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants