Skip to content

Implement getReadTimeNanos for JdbcRecordCursor#13132

Merged
raunaqmorarka merged 1 commit intotrinodb:masterfrom
posulliv:physical-read-time-jdbc
Aug 2, 2022
Merged

Implement getReadTimeNanos for JdbcRecordCursor#13132
raunaqmorarka merged 1 commit intotrinodb:masterfrom
posulliv:physical-read-time-jdbc

Conversation

@posulliv
Copy link
Copy Markdown
Contributor

@posulliv posulliv commented Jul 8, 2022

Description

Implement the getReadTimeNanos method in JdbcRecordCursor so that physicalInputReadTime will represent how long was spent reading data.

This will also show up in a in EXPLAIN ANALYZE VERBOSE output for scans on JDBC connectors. For example, with this change a TableScan operator against a MySQL table shows physical input read time in connector metrics:

 Fragment 1 [SOURCE]
     CPU: 44.27ms, Scheduled: 73.26ms, Blocked 0.00ns (Input: 0.00ns, Output: 0.00ns), Input: 1503 rows (281.36kB); per task: avg.: 1503.00 std.dev.: 0
     Output layout: [custkey, name, address, nationkey, phone, acctbal, mktsegment, comment, dummy_col]
     Output partitioning: SINGLE []
     TableScan[table = mysql:junk.customer junk.customer]
         Layout: [custkey:bigint, name:varchar(255), address:varchar(255), nationkey:bigint, phone:varchar(255), acctbal:double, mktsegment:varchar(255
         Estimates: {rows: 1483 (517.02kB), cpu: 517.02k, memory: 0B, network: 0B}
         CPU: 43.00ms (100.00%), Scheduled: 71.00ms (100.00%), Blocked: 0.00ns (?%), Output: 1503 rows (281.36kB)
         connector metrics:
           'Physical input read time' = {duration=5.20ms}

Is this change a fix, improvement, new feature, refactoring, or other?

Minor improvement.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( X) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

() No release notes entries required.
(x) Release notes entries required with the following suggested text:

# ClickHouse, Druid, MariaDb, MySql, Oracle, PostgreSQL, Redshift, SingleStore, SQL server, Phoenix
* Report wall time spent reading data from JDBC sources. ({issue}`13132`)

@cla-bot cla-bot bot added the cla-signed label Jul 8, 2022
@posulliv posulliv force-pushed the physical-read-time-jdbc branch from 87c6484 to b9a94ad Compare July 10, 2022 22:41
@posulliv posulliv force-pushed the physical-read-time-jdbc branch 2 times, most recently from 756bb30 to 0df0e7e Compare August 1, 2022 20:42
@posulliv posulliv marked this pull request as ready for review August 1, 2022 20:43
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.

lgtm % comment

@posulliv posulliv force-pushed the physical-read-time-jdbc branch from 0df0e7e to 679ffed Compare August 1, 2022 21:11
@raunaqmorarka raunaqmorarka merged commit 737b614 into trinodb:master Aug 2, 2022
@github-actions github-actions bot added this to the 392 milestone Aug 2, 2022
@posulliv posulliv deleted the physical-read-time-jdbc branch August 2, 2022 12:40
@colebow
Copy link
Copy Markdown
Member

colebow commented Aug 2, 2022

Would it be reasonable to put this release note in the JDBC driver section rather than all of the JDBC-based connectors?

@raunaqmorarka
Copy link
Copy Markdown
Member

Would it be reasonable to put this release note in the JDBC driver section rather than all of the JDBC-based connectors?

This is not related to the Trino JDBC driver. It's about JDBC-based connectors reporting the wall time spent in reading data from the external system to the engine so that it can be shown in EXPLAIN ANALYZE VERBOSE, web ui and reported to the event listeners.

@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 2, 2022

JdbcRecordCursor is part of JDBC-based connectors
and it's not part of JDBC driver (the two things are independent)

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