You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "zhuyaogai (via GitHub)" <gi...@apache.org> on 2023/10/20 13:22:29 UTC
[PR] [KYUUBI #5382] Refactor JDBC engine to reduce to code duplication. [kyuubi]
zhuyaogai opened a new pull request, #5490:
URL: https://github.com/apache/kyuubi/pull/5490
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
To fix https://github.com/apache/kyuubi/issues/5382
### _How was this patch tested?_
- [ ] 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.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request
### _Was this patch authored or co-authored using generative AI tooling?_
<!--
If a generative AI tooling has been used in the process of authoring this patch, please include
phrase 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No
--
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
Re: [PR] [KYUUBI #5382] Refactor JDBC engine to reduce to code duplication. [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#discussion_r1367045896
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/JdbcOperation.scala:
##########
@@ -98,15 +98,15 @@ abstract class JdbcOperation(session: Session) extends AbstractOperation(session
override protected def afterRun(): Unit = {}
protected def toTRowSet(taken: Iterator[Row]): TRowSet = {
- val rowSetHelper = dialect.getRowSetHelper()
+ val rowSetHelper = dialect.getRowSetHelper
Review Comment:
Leave the blank quote to represent the calling rather than a value.
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/JdbcOperation.scala:
##########
@@ -98,15 +98,15 @@ abstract class JdbcOperation(session: Session) extends AbstractOperation(session
override protected def afterRun(): Unit = {}
protected def toTRowSet(taken: Iterator[Row]): TRowSet = {
- val rowSetHelper = dialect.getRowSetHelper()
+ val rowSetHelper = dialect.getRowSetHelper
rowSetHelper.toTRowSet(
taken.toList.map(_.values),
schema.columns,
getProtocolVersion)
}
override def getResultSetMetadata: TGetResultSetMetadataResp = {
- val schemaHelper = dialect.getSchemaHelper()
+ val schemaHelper = dialect.getSchemaHelper
Review Comment:
ditto.
--
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
Re: [PR] [KYUUBI #5382] Refactor JDBC engine to reduce to code duplication. [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#discussion_r1367049297
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/schema/RowSetHelper.scala:
##########
@@ -43,7 +51,7 @@ abstract class RowSetHelper {
val columnSize = row.size
var j = 0
while (j < columnSize) {
- val columnValue = toTColumnValue(j, row, columns)
+ val columnValue = toTColumnValue(row, j, columns)
Review Comment:
No need to change the signature of method `toTColumnValue`. The param `ordinal` is also used in first place in the implementation.
--
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
Re: [PR] [KYUUBI #5382] Refactor JDBC engine to reduce to code duplication. [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#discussion_r1367044169
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/JdbcDialect.scala:
##########
@@ -30,21 +30,34 @@ import org.apache.kyuubi.util.reflect.ReflectUtils._
abstract class JdbcDialect extends SupportServiceLoader with Logging {
- def createStatement(connection: Connection, fetchSize: Int = 1000): Statement
+ def createStatement(connection: Connection): Statement = {
Review Comment:
Would prefer the original signature. Take `fetchSize` as input param for decoupling.
--
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
Re: [PR] [KYUUBI #5382][JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#issuecomment-1775317442
Thanks for the contribution from @zhuyaogai and the review by @zhaomin1423 .
Merged to master(1.9.0).
--
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
Re: [PR] [KYUUBI #5382][JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 closed pull request #5490: [KYUUBI #5382][JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers
URL: https://github.com/apache/kyuubi/pull/5490
--
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
Re: [PR] [KYUUBI #5382] [JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers [kyuubi]
Posted by "zhuyaogai (via GitHub)" <gi...@apache.org>.
zhuyaogai commented on PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#issuecomment-1773672740
@bowenliang123 hi, thanks for your suggestions, I have made all the changes. Please review it again when you have time :).
--
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
Re: [PR] [KYUUBI #5382] [JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#issuecomment-1772901079
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5490?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5490](https://app.codecov.io/gh/apache/kyuubi/pull/5490?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4ad6b3c) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/c4cdf18aad213c1de030cc5312487340c4074f7b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c4cdf18) will **not change** coverage.
> Report is 1 commits behind head on master.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #5490 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 588 588
Lines 33466 33466
Branches 4401 4401
======================================
Misses 33466 33466
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
Re: [PR] [KYUUBI #5382] [JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers [kyuubi]
Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#discussion_r1367912968
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/schema/RowSetHelper.scala:
##########
@@ -95,4 +165,160 @@ abstract class RowSetHelper {
}
ret
}
+
+ protected def toDefaultTColumn(rows: Seq[Seq[Any]], ordinal: Int, sqlType: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val rowSize = rows.length
+ val values = new util.ArrayList[String](rowSize)
+ var i = 0
+ while (i < rowSize) {
+ val row = rows(i)
+ nulls.set(i, row(ordinal) == null)
+ val value =
+ if (row(ordinal) == null) {
+ ""
+ } else {
+ toHiveString(row(ordinal), sqlType)
+ }
+ values.add(value)
+ i += 1
+ }
+ TColumn.stringVal(new TStringColumn(values, nulls))
+ }
+
+ protected def toBitTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[java.lang.Boolean](rows, ordinal, nulls, true)
+ TColumn.boolVal(new TBoolColumn(values, nulls))
+ }
+
+ protected def toTinyIntTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[java.lang.Byte](rows, ordinal, nulls, 0.toByte)
+ TColumn.byteVal(new TByteColumn(values, nulls))
+ }
+
+ protected def toSmallIntTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[java.lang.Short](rows, ordinal, nulls, 0.toShort)
+ TColumn.i16Val(new TI16Column(values, nulls))
+ }
+
+ protected def toIntegerTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[java.lang.Integer](rows, ordinal, nulls, 0)
+ TColumn.i32Val(new TI32Column(values, nulls))
+ }
+
+ protected def toBigIntTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[lang.Long](rows, ordinal, nulls, 0L)
+ TColumn.i64Val(new TI64Column(values, nulls))
+ }
+
+ protected def toRealTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[lang.Float](rows, ordinal, nulls, 0.toFloat)
+ .asScala.map(n => java.lang.Double.valueOf(n.toString)).asJava
+ TColumn.doubleVal(new TDoubleColumn(values, nulls))
+ }
+
+ protected def toDoubleTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[lang.Double](rows, ordinal, nulls, 0.toDouble)
+ TColumn.doubleVal(new TDoubleColumn(values, nulls))
+ }
+
+ protected def toCharTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ toVarcharTColumn(rows, ordinal)
+ }
+
+ protected def toVarcharTColumn(rows: Seq[Seq[Any]], ordinal: Int): TColumn = {
+ val nulls = new java.util.BitSet()
+ val values = getOrSetAsNull[String](rows, ordinal, nulls, "")
+ TColumn.stringVal(new TStringColumn(values, nulls))
+ }
+
+ // ==========================================================
+
+ protected def toBitTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val boolValue = new TBoolValue
+ if (row(ordinal) != null) boolValue.setValue(row(ordinal).asInstanceOf[Boolean])
+ TColumnValue.boolVal(boolValue)
+ }
+
+ protected def toTinyIntTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val byteValue = new TByteValue
+ if (row(ordinal) != null) byteValue.setValue(row(ordinal).asInstanceOf[Byte])
+ TColumnValue.byteVal(byteValue)
+ }
+
+ protected def toSmallIntTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val tI16Value = new TI16Value
+ if (row(ordinal) != null) tI16Value.setValue(row(ordinal).asInstanceOf[Short])
+ TColumnValue.i16Val(tI16Value)
+ }
+
+ protected def toIntegerTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val tI32Value = new TI32Value
+ if (row(ordinal) != null) tI32Value.setValue(row(ordinal).asInstanceOf[Int])
+ TColumnValue.i32Val(tI32Value)
+ }
+
+ protected def toBigIntTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val tI64Value = new TI64Value
+ if (row(ordinal) != null) tI64Value.setValue(row(ordinal).asInstanceOf[Long])
+ TColumnValue.i64Val(tI64Value)
+ }
+
+ protected def toRealTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val tDoubleValue = new TDoubleValue
+ if (row(ordinal) != null) {
+ val doubleValue = java.lang.Double.valueOf(row(ordinal).asInstanceOf[Float].toString)
+ tDoubleValue.setValue(doubleValue)
+ }
+ TColumnValue.doubleVal(tDoubleValue)
+ }
+
+ protected def toDoubleTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val tDoubleValue = new TDoubleValue
+ if (row(ordinal) != null) tDoubleValue.setValue(row(ordinal).asInstanceOf[Double])
+ TColumnValue.doubleVal(tDoubleValue)
+ }
+
+ protected def toCharTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ toVarcharTColumnValue(row, ordinal)
+ }
+
+ protected def toVarcharTColumnValue(row: List[Any], ordinal: Int): TColumnValue = {
+ val tStringValue = new TStringValue
+ if (row(ordinal) != null) tStringValue.setValue(row(ordinal).asInstanceOf[String])
+ TColumnValue.stringVal(tStringValue)
+ }
+
+ protected def toDefaultTColumnValue(
+ row: List[Any],
+ ordinal: Int,
+ types: List[Column]): TColumnValue = {
+ val tStrValue = new TStringValue
+ if (row(ordinal) != null) {
+ tStrValue.setValue(
+ toHiveString(row(ordinal), types(ordinal).sqlType))
+ }
+ TColumnValue.stringVal(tStrValue)
+ }
+
+ protected def toHiveString(data: Any, sqlType: Int): String = {
+ (data, sqlType) match {
+ case (date: Date, Types.DATE) =>
+ formatDate(date)
+ case (dateTime: LocalDateTime, Types.TIMESTAMP) =>
+ formatLocalDateTime(dateTime)
+ case (decimal: java.math.BigDecimal, Types.DECIMAL) =>
+ decimal.toPlainString
+ // TODO support bitmap and hll
Review Comment:
It can be removed, this is a comment for doris.
--
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
Re: [PR] [KYUUBI #5382] Refactor JDBC engine to reduce to code duplication. [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5490:
URL: https://github.com/apache/kyuubi/pull/5490#discussion_r1367040214
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/schema/RowSetHelper.scala:
##########
@@ -16,12 +16,20 @@
*/
package org.apache.kyuubi.engine.jdbc.schema
-import java.util
+import java.{lang, util}
+import java.sql.{Date, Types}
+import java.time.LocalDateTime
+
+import scala.collection.JavaConverters._
import org.apache.hive.service.rpc.thrift._
+import org.apache.kyuubi.util.RowSetUtils.{bitSetToBuffer, formatDate, formatLocalDateTime}
+
abstract class RowSetHelper {
+ val nulls = new java.util.BitSet()
Review Comment:
Move back into method as local variable.
--
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