You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/05/24 07:58:28 UTC
[GitHub] [incubator-kyuubi] wolfboys opened a new pull request, #2730: [KYUUBI #2238] Support Flink 1.15
wolfboys opened a new pull request, #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730
### Why are the changes needed?
Flink 1.15.0 is out, Kyuubi needs to support it.
Please clarify why the changes are needed. For instance
1. new add flink 1.15 support
2. new add `flink-1.14` and `flink-1.15` maven profiles to support both Flink 1.14 & 1.15
flink 1.14 all functions and codes are kept unchanged without any impact
flink 1.15 support part of the work is still in progress, At present, 32 test cases have passed and 5 have failed, You can review, put forward suggestions for improvement, I will continue to improve.
### how to build with flink 1.14
```
mvn clean install -DskipTests -Pflink-1.14
```
### how to build with flink 1.15
```
mvn clean install -DskipTests -Pflink-1.15
```
### how to run test cases with flink 1.14 in idea?
default flink version is 1.14, You can run test cases in idea
### how to run test cases with flink 1.15 in idea?
1. locate the `flink.version` in the parent module properties modify to 1.15.0
2. locate the `flink.module.scala.suffix` in the parent module properties modify to empty string
3. run the test cases in idea
```
<flink.version>1.15.0</flink.version>
<flink.module.scala.suffix></flink.module.scala.suffix>
```
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] wolfboys commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
wolfboys commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880314782
##########
pom.xml:
##########
@@ -1330,19 +1331,19 @@
<dependency>
<groupId>org.apache.flink</groupId>
- <artifactId>flink-yarn_${scala.binary.version}</artifactId>
+ <artifactId>flink-yarn${flink.module.scala.suffix}</artifactId>
<version>${flink.version}</version>
</dependency>
Review Comment:
> This dependency seems does use anymore, we can remove it.
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880234627
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
Review Comment:
Would you please add comments here? e.g. FLINK-xxx changes the return type from xx to yy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#issuecomment-1135618134
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2730](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0616e74) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/5b9d92e96cad7e0a91b13328c46225f3dd7adcbb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b9d92e) will **decrease** coverage by `0.20%`.
> The diff coverage is `8.00%`.
```diff
@@ Coverage Diff @@
## master #2730 +/- ##
============================================
- Coverage 62.03% 61.83% -0.21%
Complexity 238 238
============================================
Files 407 407
Lines 19626 19694 +68
Branches 2635 2656 +21
============================================
+ Hits 12175 12177 +2
- Misses 6289 6357 +68
+ Partials 1162 1160 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../apache/kyuubi/engine/flink/FlinkEngineUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9GbGlua0VuZ2luZVV0aWxzLnNjYWxh) | `12.00% <0.00%> (-0.50%)` | :arrow_down: |
| [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `47.61% <8.45%> (-36.80%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoSqlEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3FsRW5naW5lLnNjYWxh) | `52.63% <0.00%> (-7.02%)` | :arrow_down: |
| [...la/org/apache/kyuubi/operation/FetchIterator.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vRmV0Y2hJdGVyYXRvci5zY2FsYQ==) | `93.02% <0.00%> (-3.76%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.64% <0.00%> (-0.12%)` | :arrow_down: |
| [...che/kyuubi/engine/trino/operation/GetSchemas.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL29wZXJhdGlvbi9HZXRTY2hlbWFzLnNjYWxh) | `85.00% <0.00%> (ø)` | |
| [...he/kyuubi/engine/trino/operation/GetCatalogs.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL29wZXJhdGlvbi9HZXRDYXRhbG9ncy5zY2FsYQ==) | `90.00% <0.00%> (ø)` | |
| [...kyuubi/engine/trino/operation/TrinoOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL29wZXJhdGlvbi9Ucmlub09wZXJhdGlvbi5zY2FsYQ==) | `53.06% <0.00%> (ø)` | |
| [...uubi/engine/trino/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL29wZXJhdGlvbi9FeGVjdXRlU3RhdGVtZW50LnNjYWxh) | `81.48% <0.00%> (+0.99%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `70.68% <0.00%> (+1.72%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5b9d92e...0616e74](https://codecov.io/gh/apache/incubator-kyuubi/pull/2730?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] link3280 commented on a diff in pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
link3280 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880384552
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
IIUC, `RowData` is Flink's internal data type, thus I think `Row` is more suitable for our case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] wolfboys commented on pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
wolfboys commented on PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#issuecomment-1136087330
@SteNicholas @deadwind4 PTAL. thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880469226
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
Are we suffering from [FLINK-21456](https://issues.apache.org/jira/browse/FLINK-21456)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880237206
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
From my understanding, `RowData` is more future proof than `Row`, then `convertToDataRow` should more reasonable?
WDYT? @link3280 @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880237206
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
From my understanding, `RowData` is more future proof than `Row`, then `convertToRowData` should more reasonable?
WDYT? @link3280 @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#issuecomment-1136686308
Thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 closed pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
URL: https://github.com/apache/incubator-kyuubi/pull/2730
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880237206
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
From my understanding, `RowData` is more future proof than `Row`, then `convertToRowData` should more reasonable?
WDYT? @link3280 @SteNicholas @deadwind4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880237206
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
From my understanding, `RowData` is more future proof than `Row`, then `convertToDataRow` should more reasonable?
@link3280 @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2730: [KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r880233174
##########
pom.xml:
##########
@@ -1330,19 +1331,19 @@
<dependency>
<groupId>org.apache.flink</groupId>
- <artifactId>flink-yarn_${scala.binary.version}</artifactId>
+ <artifactId>flink-yarn${flink.module.scala.suffix}</artifactId>
<version>${flink.version}</version>
</dependency>
Review Comment:
This dependency seems does use anymore, we can remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] deadwind4 commented on a diff in pull request #2730: [WIP][KYUUBI #2238] Support Flink 1.15
Posted by GitBox <gi...@apache.org>.
deadwind4 commented on code in PR #2730:
URL: https://github.com/apache/incubator-kyuubi/pull/2730#discussion_r881141947
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
I talk with Cheng Pan offline, LGTM.
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
We can directly convert RowData to String like this.
https://github.com/apache/flink/blob/8cbb42a839919c3163f30928254a1afd0077f6a6/flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableResultView.java#L306-L309
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
We can reuse some methods and classes in FLINK-21456, for example, the `getRowDataStringConverter` method.
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala:
##########
@@ -142,7 +151,18 @@ class ExecuteStatement(
case TypedResult.ResultType.PAYLOAD =>
(1 to result.getPayload).foreach { page =>
if (rows.size < resultMaxRows) {
- rows ++= executor.retrieveResultPage(resultId, page).asScala
+ val result = executor.retrieveResultPage(resultId, page).asScala.toList
+ result.headOption match {
+ case None =>
+ case Some(r) =>
+ // for flink 1.14
+ if (r.getClass == classOf[Row]) {
+ rows ++= result.asInstanceOf[List[Row]]
+ } else {
+ // for flink 1.15+
+ rows ++= result.map(r => convertToRow(r.asInstanceOf[RowData], dataTypes))
Review Comment:
The Flink native print is directly to convert RowData to String.
Like this jira issue
https://issues.apache.org/jira/browse/FLINK-24461
Stay native, Stay simple.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org