Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@

package org.apache.spark.sql.hive.thriftserver

import java.util.UUID
import java.util.regex.Pattern

import scala.collection.JavaConverters.seqAsJavaListConverter

import org.apache.commons.lang3.exception.ExceptionUtils
import org.apache.hadoop.hive.ql.security.authorization.plugin.{HiveOperationType, HivePrivilegeObject}
import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject.HivePrivilegeObjectType
import org.apache.hive.service.cli._
Expand All @@ -34,7 +32,7 @@ import org.apache.spark.sql.SQLContext
import org.apache.spark.sql.catalyst.TableIdentifier
import org.apache.spark.sql.catalyst.catalog.SessionCatalog
import org.apache.spark.sql.hive.thriftserver.ThriftserverShimUtils.toJavaSQLType
import org.apache.spark.sql.types.StructType
import org.apache.spark.sql.types._

/**
* Spark's own SparkGetColumnsOperation
Expand Down Expand Up @@ -126,12 +124,52 @@ private[hive] class SparkGetColumnsOperation(
HiveThriftServer2.eventManager.onStatementFinish(statementId)
}

/**
* For numeric and datetime types, it returns the default size of its catalyst type
* For struct type, when its elements are fixed-size, the summation of all element sizes will be
* returned.
* For array, map, string, and binaries, the column size is variable, return null as unknown.
*/
private def getColumnSize(typ: DataType): Option[Int] = typ match {

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.

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.

Hive does not return same result for each type

case StringType | BinaryType | _: ArrayType | _: MapType => None
case StructType(fields) =>
val sizeArr = fields.map(f => getColumnSize(f.dataType))
if (sizeArr.contains(None)) {
None
} else {
Some(sizeArr.map(_.get).sum)
}
case other => Some(other.defaultSize)

@cloud-fan cloud-fan Aug 3, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I think it's safer to list the types we know the size, instead of listing the types we don't know the size. I'd prefer

case dt @ (_: NumericType | DateType | TimestampType) => dt.defaultSize
... 

}

/**
* The number of fractional digits for this type.
* Null is returned for data types where this is not applicable.
* For boolean and integrals, the decimal digits is 0
* For floating types, we follow the IEEE Standard for Floating-Point Arithmetic (IEEE 754)
* For timestamp values, we support microseconds
* For decimals, it returns the scale
*/
private def getDecimalDigits(typ: DataType) = typ match {
case BooleanType | _: IntegerType => Some(0)
case FloatType => Some(7)
case DoubleType => Some(15)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how do we pick these numbers?

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.

They are from hive's TypeDescriptor and I also checked the results, which are same from hive and spark, e.g. select 0.12345678901234567890D

@yaooqinn yaooqinn Jul 31, 2020

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.

the result is 0.12345678901234568 which have 17 decimal digits but hive says it's 15 for double...

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.

It seems to follow IEEE 754 https://en.wikipedia.org/wiki/IEEE_754

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a code comment to explain it?

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.

OK

case d: DecimalType => Some(d.scale)
case TimestampType => Some(6)
case _ => None
}

private def getNumPrecRadix(typ: DataType): Option[Int] = typ match {
case _: NumericType => Some(10)
case _ => None
}

private def addToRowSet(
columnPattern: Pattern,
dbName: String,
tableName: String,
schema: StructType): Unit = {
schema.foreach { column =>
schema.zipWithIndex.foreach { case (column, pos) =>
if (columnPattern != null && !columnPattern.matcher(column.name).matches()) {
} else {
val rowData = Array[AnyRef](
Expand All @@ -141,17 +179,17 @@ private[hive] class SparkGetColumnsOperation(
column.name, // COLUMN_NAME
toJavaSQLType(column.dataType.sql).asInstanceOf[AnyRef], // DATA_TYPE
column.dataType.sql, // TYPE_NAME
null, // COLUMN_SIZE
getColumnSize(column.dataType).map(_.asInstanceOf[AnyRef]).orNull, // COLUMN_SIZE
Comment thread
yaooqinn marked this conversation as resolved.
null, // BUFFER_LENGTH, unused
null, // DECIMAL_DIGITS
null, // NUM_PREC_RADIX
getDecimalDigits(column.dataType).map(_.asInstanceOf[AnyRef]).orNull, // DECIMAL_DIGITS
getNumPrecRadix(column.dataType).map(_.asInstanceOf[AnyRef]).orNull, // NUM_PREC_RADIX
(if (column.nullable) 1 else 0).asInstanceOf[AnyRef], // NULLABLE
column.getComment().getOrElse(""), // REMARKS
null, // COLUMN_DEF
null, // SQL_DATA_TYPE
null, // SQL_DATETIME_SUB
null, // CHAR_OCTET_LENGTH
null, // ORDINAL_POSITION
pos.asInstanceOf[AnyRef], // ORDINAL_POSITION
"YES", // IS_NULLABLE

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this respect column.nullable?

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.

IS_NULLABLE is related to PK, "YES" here is more specific as we don't have such thing implemented

null, // SCOPE_CATALOG
null, // SCOPE_SCHEMA
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@ package org.apache.spark.sql.hive.thriftserver

import java.sql.SQLException

import scala.collection.JavaConverters._

import org.apache.hive.service.cli.HiveSQLException

import org.apache.spark.sql.hive.HiveUtils
import org.apache.spark.sql.types.{ByteType, DecimalType, DoubleType, LongType, ShortType}

trait ThriftServerWithSparkContextSuite extends SharedThriftServer {

Expand Down Expand Up @@ -101,6 +104,79 @@ trait ThriftServerWithSparkContextSuite extends SharedThriftServer {
}
}
}

test("SparkGetColumnsOperation") {

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.

Please make this test title clearer, too.

val schemaName = "default"
val tableName = "spark_get_col_operation"
val decimalType = DecimalType(10, 2)
val ddl =
s"""
|CREATE TABLE $schemaName.$tableName

@maropu maropu Jul 31, 2020

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 think we don't have a strict rule for the format though, how about following the existing format?

s"""CREATE TABLE ddl_test (
| a STRING,
| b STRING,
| `extra col` ARRAY<INT>,
| `<another>` STRUCT<x: INT, y: ARRAY<BOOLEAN>>
|)
|USING json

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.

OK, this one looks better.

| (
| a boolean comment '0',

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.

Could you test all the types where possible for test coverage?

| b int comment '1',
| c float comment '2',
| d ${decimalType.sql} comment '3',
| e array<long> comment '4',
| f array<string> comment '5',
| g map<smallint, tinyint> comment '6',
| h date comment '7',
| i timestamp comment '8',
| j struct<X: bigint,Y: double> comment '9'
| ) using parquet""".stripMargin

withCLIServiceClient { client =>
val sessionHandle = client.openSession(user, "")
val confOverlay = new java.util.HashMap[java.lang.String, java.lang.String]
val opHandle = client.executeStatement(sessionHandle, ddl, confOverlay)
var status = client.getOperationStatus(opHandle)
while (!status.getState.isTerminal) {
Thread.sleep(10)
status = client.getOperationStatus(opHandle)
}
val getCol = client.getColumns(sessionHandle, "", schemaName, tableName, null)

@dongjoon-hyun dongjoon-hyun Aug 4, 2020

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.

Hi, @yaooqinn and @cloud-fan . I'm not sure but this new test case seems to break all Jenkins Maven jobs on master branch. Could you take a look?

org.apache.hive.service.cli.HiveSQLException: java.lang.RuntimeException: org.apache.hadoop.hive.ql.metadata.HiveException: java.lang.ClassCastException: org.apache.hadoop.hive.ql.security.HadoopDefaultAuthenticator cannot be cast to org.apache.hadoop.hive.ql.security.HiveAuthenticationProvider
      at org.apache.hive.service.cli.thrift.ThriftCLIServiceClient.checkStatus(ThriftCLIServiceClient.java:42)
      at org.apache.hive.service.cli.thrift.ThriftCLIServiceClient.getColumns(ThriftCLIServiceClient.java:275)
      at org.apache.spark.sql.hive.thriftserver.ThriftServerWithSparkContextSuite.$anonfun$$init$$17(ThriftServerWithSparkContextSuite.scala:146)

From the screenshot, please ignore SBT failures.
Screen Shot 2020-08-03 at 5 20 21 PM

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.

Let's revert if it takes a while to fix.

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.

Looks like something related to another version of Hive which belong to another job(maybe failed) not being cleaned up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not all maven builds are broken:
image

Let's wait a little bit and see if it's an env issue.

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.

Ya. It seems that Hive 2.3 is affected. And, JDK11 maven seems not running those test suite.
Do we have a follow-up idea, @yaooqinn and @cloud-fan ?
The error message is clear, java.lang.ClassCastException. If there is no idea, I'd like to recommend to revert first and pass Maven Jenkins properly.

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.

BTW, this is not a flaky failure. This broke some profiles consistently.

@dongjoon-hyun dongjoon-hyun Aug 4, 2020

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.

BTW, it's up to you, @cloud-fan . I'll not revert this by myself since PRBuilder is still working.

val rowSet = client.fetchResults(getCol)
val columns = rowSet.toTRowSet.getColumns
assert(columns.get(0).getStringVal.getValues.asScala.forall(_.isEmpty),
"catalog name mismatches")

assert(columns.get(1).getStringVal.getValues.asScala.forall(_ == schemaName),
"schema name mismatches")

assert(columns.get(2).getStringVal.getValues.asScala.forall(_ == tableName),
"table name mismatches")

// column name
columns.get(3).getStringVal.getValues.asScala.zipWithIndex.foreach {
case (v, i) => assert(v === ('a' + i).toChar.toString, "column name mismatches")
}

val javaTypes = columns.get(4).getI32Val.getValues
assert(javaTypes.get(3).intValue() === java.sql.Types.DECIMAL)
assert(javaTypes.get(6).intValue() === java.sql.Types.JAVA_OBJECT)

val typeNames = columns.get(5).getStringVal.getValues
assert(typeNames.get(3) === decimalType.sql)

val colSize = columns.get(6).getI32Val.getValues
assert(colSize.get(3).intValue() === decimalType.defaultSize)

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.

Could you check all the elements in colSize? (Rather, I think we need to check all elements in the fetched rowSet.)

assert(colSize.get(6).intValue() === 0)
assert(colSize.get(9).intValue() === LongType.defaultSize + DoubleType.defaultSize,
"struct column size mismatches")

val decimalDigits = columns.get(8).getI32Val.getValues
assert(decimalDigits.get(1).intValue() === 0)
assert(decimalDigits.get(2).intValue() === 7)
assert(decimalDigits.get(3).intValue() === decimalType.scale)
assert(decimalDigits.get(8).intValue() === 6,
"timestamp support 6 digits for second fraction")

val positions = columns.get(16).getI32Val.getValues
positions.asScala.zipWithIndex.foreach { case (pos, idx) =>
assert(pos === idx, "the client columns disorder")
}
}
}
}


Expand Down