Generalize TRowSet generators #5851

bowenliang123 · 2023-12-13T03:01:26Z

🔍 Description

Issue References 🔗

As described.

Describe Your Solution 🔧

Introduced a generalized RowSet generator AbstractTRowSetGenerator[SchemaT, RowT, ColumnT]
- extract common methods for looping and assembling the rows to TRowSet
- support generation for either column-based or row-based TRowSet
Each engine creates a sub-generator of AbstractTRowSetGenerator
- focus on mapping and conversion from the engine's data type to the relative Thrift type
- implements the schema data type and column value methods
- create a generator instance instead of the previously used RowSet object, for isolated session-aware or thread-aware configs or context, eg. Timezone ID for Flink, and the Hive time formatters for Spark.
This PR covers the TRowSet generation for the server and the engines of Spark/Flink/Trino/Chat, except the JDBC engine which will be supported in the follow-ups with JDBC dialect support.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

No behavior changes.

Behavior With This Pull Request 🎉

No behavior changes.

Related Unit Tests

CI tests.

Checklists

📝 Author Self Checklist

My code follows the style guidelines of this project
I have performed a self-review
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

Be nice. Be informative.

codecov-commenter · 2023-12-14T01:04:34Z

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (a0fdead) 61.28% compared to head (1d2f73a) 61.35%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
...che/kyuubi/sql/schema/ServerTRowSetGenerator.scala	28.12%	21 Missing and 2 partials ⚠️
...gine/spark/schema/SparkArrowTRowSetGenerator.scala	47.82%	8 Missing and 4 partials ⚠️
...yuubi/engine/schema/AbstractTRowSetGenerator.scala	93.80%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5851      +/-   ##
============================================
+ Coverage     61.28%   61.35%   +0.07%     
  Complexity       23       23              
============================================
  Files           609      612       +3     
  Lines         36282    36238      -44     
  Branches       4976     4967       -9     
============================================
- Hits          22235    22234       -1     
+ Misses        11655    11601      -54     
- Partials       2392     2403      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pan3793 · 2023-12-15T05:59:40Z

...kyuubi-chat-engine/src/main/scala/org/apache/kyuubi/engine/chat/schema/RowSetGenerator.scala

+import org.apache.kyuubi.shaded.hive.service.rpc.thrift._
+import org.apache.kyuubi.shaded.hive.service.rpc.thrift.TTypeId._
+
+class RowSetGenerator


can we have a different name for each module? e.g. ChatRowSetGenerator TrinoRowSetGenerator, KyuubiRowSetGenerator(for server module)

Previously, each module had the same class RowSet for TRowSet generation. As RowSetGenerator follows the same style and it's not shared crossing the modules, it's not required to put the engine name as part of the class name.

this actually confuses developers when searching the code base with RowSetGenerator

OK, I have added the engine name as a class name prefix.

pan3793 · 2023-12-15T06:01:52Z

...yuubi-chat-engine/src/main/scala/org/apache/kyuubi/engine/chat/operation/ChatOperation.scala

-    val taken = iter.take(rowSetSize)
-    val resultRowSet = RowSet.toTRowSet(taken.toSeq, 1, getProtocolVersion)
+    val taken = iter.take(rowSetSize).map(_.toSeq)
+    val resultRowSet = new RowSetGenerator().toTRowSet(


why should create a new instance each time? is it stateful? if not, I suppose we can define object RowSetGenerator instead of class RowSetGenerator

It's designed on purpose. As for Spark's RowSetGenerator, we will have TimeFormatters instance inside, and should not be shared by others as they may not be thread-safe. As for Flink's RowSetGenerator, we init the RowSetGenerator with the timezone id instance.

that makes sense. thanks for explanation

kyuubi-common/src/main/scala/org/apache/kyuubi/engine/schema/AbstractRowSetGenerator.scala

bowenliang123 · 2023-12-15T09:44:29Z

Thanks, merged to master (1.9.0).

…ects # 🔍 Description ## Issue References 🔗 As described. ## Describe Your Solution 🔧 - Introduced JdbcTRowSetGenerator extending `AbstractTRowSetGenerator ` introduced in #5851 in JDBC engine. - Provide a DefaultJdbcTRowSetGenerator as default implementation for mapping the JDBC data types to TRowSet generation - Make JDBC dialect providing TRowSetGenerator extending DefaultJdbcTRowSetGenerator to adapt detailed differences ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes #5861 from bowenliang123/jdbc-rowgen. Closes #5861 7f8658d [Bowen Liang] generalize jdbc TRowSet generator Authored-by: Bowen Liang <[email protected]> Signed-off-by: liangbowen <[email protected]>

# 🔍 Description ## Issue References 🔗 As described. ## Describe Your Solution 🔧 - Introduced a generalized RowSet generator `AbstractTRowSetGenerator[SchemaT, RowT, ColumnT]` - extract common methods for looping and assembling the rows to TRowSet - support generation for either column-based or row-based TRowSet - Each engine creates a sub-generator of `AbstractTRowSetGenerator` - focus on mapping and conversion from the engine's data type to the relative Thrift type - implements the schema data type and column value methods - create a generator instance instead of the previously used `RowSet` object, for isolated session-aware or thread-aware configs or context, eg. Timezone ID for Flink, and the Hive time formatters for Spark. - This PR covers the TRowSet generation for the server and the engines of Spark/Flink/Trino/Chat, except the JDBC engine which will be supported in the follow-ups with JDBC dialect support. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ No behavior changes. #### Behavior With This Pull Request 🎉 No behavior changes. #### Related Unit Tests CI tests. --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes apache#5851 from bowenliang123/rowset-gen. Closes apache#5851 1d2f73a [Bowen Liang] common RowSetGenerator Authored-by: Bowen Liang <[email protected]> Signed-off-by: Bowen Liang <[email protected]>

…h dialects # 🔍 Description ## Issue References 🔗 As described. ## Describe Your Solution 🔧 - Introduced JdbcTRowSetGenerator extending `AbstractTRowSetGenerator ` introduced in apache#5851 in JDBC engine. - Provide a DefaultJdbcTRowSetGenerator as default implementation for mapping the JDBC data types to TRowSet generation - Make JDBC dialect providing TRowSetGenerator extending DefaultJdbcTRowSetGenerator to adapt detailed differences ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes apache#5861 from bowenliang123/jdbc-rowgen. Closes apache#5861 7f8658d [Bowen Liang] generalize jdbc TRowSet generator Authored-by: Bowen Liang <[email protected]> Signed-off-by: liangbowen <[email protected]>

github-actions bot added module:spark module:common module:flink module:trino module:jdbc module:server and removed module:jdbc labels Dec 13, 2023

bowenliang123 force-pushed the rowset-gen branch from 2990665 to 8af2825 Compare December 13, 2023 14:39

bowenliang123 changed the title ~~Extact common RowSetGenerator for engines~~ Generalized RowSet generator Dec 13, 2023

bowenliang123 marked this pull request as ready for review December 13, 2023 15:44

bowenliang123 requested a review from pan3793 December 14, 2023 01:57

bowenliang123 force-pushed the rowset-gen branch 2 times, most recently from 37c2bf6 to 8f5ebe9 Compare December 15, 2023 03:19

pan3793 reviewed Dec 15, 2023

View reviewed changes

pan3793 approved these changes Dec 15, 2023

View reviewed changes

bowenliang123 changed the title ~~Generalized RowSet generator~~ Generalize TRowSet generators Dec 15, 2023

bowenliang123 force-pushed the rowset-gen branch from aabfea9 to d52d6ac Compare December 15, 2023 07:39

common RowSetGenerator

1d2f73a

bowenliang123 force-pushed the rowset-gen branch from 3936f4e to 1d2f73a Compare December 15, 2023 08:05

bowenliang123 self-assigned this Dec 15, 2023

bowenliang123 added this to the v1.9.0 milestone Dec 15, 2023

bowenliang123 closed this in 9047151 Dec 15, 2023

bowenliang123 deleted the rowset-gen branch December 15, 2023 09:44

This was referenced Dec 16, 2023

Generalize TRowSet generator for JDBC engine with dialects #5861

Closed

[Umbrella] Improvements and evaluation for TRowSet generation of Spark Engine #5808

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize TRowSet generators #5851

Generalize TRowSet generators #5851

bowenliang123 commented Dec 13, 2023 •

edited

Loading

codecov-commenter commented Dec 14, 2023 •

edited

Loading

pan3793 Dec 15, 2023

bowenliang123 Dec 15, 2023

pan3793 Dec 15, 2023

bowenliang123 Dec 15, 2023

pan3793 Dec 15, 2023

bowenliang123 Dec 15, 2023

pan3793 Dec 15, 2023

bowenliang123 commented Dec 15, 2023

Generalize TRowSet generators #5851

Generalize TRowSet generators #5851

Conversation

bowenliang123 commented Dec 13, 2023 • edited Loading

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

📝 Committer Pre-Merge Checklist

codecov-commenter commented Dec 14, 2023 • edited Loading

Codecov Report

pan3793 Dec 15, 2023

Choose a reason for hiding this comment

bowenliang123 Dec 15, 2023

Choose a reason for hiding this comment

pan3793 Dec 15, 2023

Choose a reason for hiding this comment

bowenliang123 Dec 15, 2023

Choose a reason for hiding this comment

pan3793 Dec 15, 2023

Choose a reason for hiding this comment

bowenliang123 Dec 15, 2023

Choose a reason for hiding this comment

pan3793 Dec 15, 2023

Choose a reason for hiding this comment

bowenliang123 commented Dec 15, 2023

bowenliang123 commented Dec 13, 2023 •

edited

Loading

codecov-commenter commented Dec 14, 2023 •

edited

Loading