You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by cs...@apache.org on 2026/07/17 08:52:36 UTC
(impala) branch master updated: IMPALA-15158: Replace ColumnType.is_binary_ with an enum
This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new cd653ccaa IMPALA-15158: Replace ColumnType.is_binary_ with an enum
cd653ccaa is described below
commit cd653ccaaca65815cf5a43ec38e4230113ab306d
Author: Csaba Ringhofer <cs...@cloudera.com>
AuthorDate: Thu Jul 9 16:16:15 2026 +0200
IMPALA-15158: Replace ColumnType.is_binary_ with an enum
This is preparation for adding new types that piggy-back
on STRING (GEOMETRY/GEOGRAPHY). The new types will be
available only in Parquet, so adding the actual types
later should not need to modify unrelated file formats.
Change-Id: I667138dac8afdd117dea8ed90a85927352f73269
Reviewed-on: http://gerrit.cloudera.org:8080/24567
Reviewed-by: Csaba Ringhofer <cs...@cloudera.com>
Tested-by: Csaba Ringhofer <cs...@cloudera.com>
---
be/src/exec/hbase/hbase-table-writer.cc | 20 ++++++----
be/src/exec/hdfs-text-table-writer.cc | 17 ++++++---
be/src/exec/iceberg-metadata/iceberg-row-reader.cc | 29 ++++++++++-----
be/src/exec/kudu/kudu-util-ir.cc | 37 +++++++++++--------
be/src/exec/paimon/paimon-jni-row-reader.cc | 17 ++++++---
be/src/exec/text-converter.inline.h | 1 +
be/src/runtime/complex-value-writer.inline.h | 1 +
be/src/runtime/types.cc | 43 +++++++++++++++++-----
be/src/runtime/types.h | 35 +++++++++++++-----
9 files changed, 136 insertions(+), 64 deletions(-)
diff --git a/be/src/exec/hbase/hbase-table-writer.cc b/be/src/exec/hbase/hbase-table-writer.cc
index 6c3d1163a..bc256e525 100644
--- a/be/src/exec/hbase/hbase-table-writer.cc
+++ b/be/src/exec/hbase/hbase-table-writer.cc
@@ -155,13 +155,19 @@ Status HBaseTableWriter::AppendRows(RowBatch* batch) {
string_value.clear();
output_expr_evals_[j]->PrintValue(value, &string_value);
const ColumnDescriptor& col_desc = table_desc_->col_descs()[j];
- if (col_desc.type().IsBinaryType()) {
- Base64Encode(string_value , &base64_encoded_value);
- data = base64_encoded_value.data();
- data_len = base64_encoded_value.length();
- } else {
- data = string_value.data();
- data_len = string_value.length();
+ StringValSubtype string_val_sub_type = col_desc.type().StringSubtype();
+ switch (string_val_sub_type) {
+ case StringValSubtype::BINARY:
+ Base64Encode(string_value , &base64_encoded_value);
+ data = base64_encoded_value.data();
+ data_len = base64_encoded_value.length();
+ break;
+ case StringValSubtype::STRING:
+ data = string_value.data();
+ data_len = string_value.length();
+ break;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << string_val_sub_type;
}
} else {
// Binary encoded
diff --git a/be/src/exec/hdfs-text-table-writer.cc b/be/src/exec/hdfs-text-table-writer.cc
index 4450dcaf8..89782117f 100644
--- a/be/src/exec/hdfs-text-table-writer.cc
+++ b/be/src/exec/hdfs-text-table-writer.cc
@@ -110,12 +110,17 @@ Status HdfsTextTableWriter::AppendRows(
PrintEscaped(&sv);
} else if (type.IsVarLenStringType()) {
const StringValue* string_value = reinterpret_cast<const StringValue*>(value);
- if (type.IsBinaryType()) {
- // TODO: try to find a more efficient implementation
- Base64Encode(
- string_value->Ptr() , string_value->Len(), &rowbatch_stringstream_);
- } else {
- PrintEscaped(string_value);
+ switch (type.StringSubtype()) {
+ case StringValSubtype::BINARY:
+ // TODO: try to find a more efficient implementation
+ Base64Encode(
+ string_value->Ptr() , string_value->Len(), &rowbatch_stringstream_);
+ break;
+ case StringValSubtype::STRING:
+ PrintEscaped(string_value);
+ break;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << type.StringSubtype();
}
} else {
output_expr_evals_[j]->PrintValue(value, &rowbatch_stringstream_);
diff --git a/be/src/exec/iceberg-metadata/iceberg-row-reader.cc b/be/src/exec/iceberg-metadata/iceberg-row-reader.cc
index bc42efa2a..06ca6dc79 100644
--- a/be/src/exec/iceberg-metadata/iceberg-row-reader.cc
+++ b/be/src/exec/iceberg-metadata/iceberg-row-reader.cc
@@ -128,12 +128,17 @@ Status IcebergRowReader::WriteSlot(JNIEnv* env, const jobject* struct_like_row,
RETURN_IF_ERROR(WriteTimeStampSlot(env, accessed_value, slot));
break;
} case TYPE_STRING: {
- if (type.IsBinaryType()) { // byte[]
- RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ true>(
- env, accessed_value, slot, tuple_data_pool));
- } else { // java.lang.String
- RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ false>(
- env, accessed_value, slot, tuple_data_pool));
+ switch (type.StringSubtype()) {
+ case StringValSubtype::BINARY: // byte[]
+ RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ true>(
+ env, accessed_value, slot, tuple_data_pool));
+ break;
+ case StringValSubtype::STRING: // java.lang.String
+ RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ false>(
+ env, accessed_value, slot, tuple_data_pool));
+ break;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << type.StringSubtype();
}
break;
} case TYPE_STRUCT: { // Struct type is not used by Impala to access values.
@@ -424,10 +429,14 @@ jclass IcebergRowReader::JavaClassFromImpalaType(const ColumnType type) {
case TYPE_TIMESTAMP: { // org.apache.iceberg.types.TimestampType
return long_cl_;
} case TYPE_STRING: {
- if (type.IsBinaryType()) { // java.nio.ByteBuffer
- return byte_buffer_cl_;
- } else { // java.lang.CharSequence
- return char_sequence_cl_;
+ switch (type.StringSubtype()) {
+ case StringValSubtype::BINARY: // java.nio.ByteBuffer
+ return byte_buffer_cl_;
+ case StringValSubtype::STRING: // java.lang.CharSequence
+ return char_sequence_cl_;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << type.StringSubtype();
+ return nullptr;
}
} case TYPE_ARRAY: { // java.util.List
return list_cl_;
diff --git a/be/src/exec/kudu/kudu-util-ir.cc b/be/src/exec/kudu/kudu-util-ir.cc
index c2aa98159..e92cbb3ab 100644
--- a/be/src/exec/kudu/kudu-util-ir.cc
+++ b/be/src/exec/kudu/kudu-util-ir.cc
@@ -68,22 +68,27 @@ Status WriteKuduValue(int col, const ColumnType& col_type, const void* value,
case TYPE_STRING: {
const StringValue* sv = reinterpret_cast<const StringValue*>(value);
kudu::Slice slice(reinterpret_cast<uint8_t*>(sv->Ptr()), sv->Len());
- if (col_type.IsBinaryType()) {
- if (copy_strings) {
- KUDU_RETURN_IF_ERROR(
- row->SetBinary(col, slice), VALUE_ERROR_MSG);
- } else {
- KUDU_RETURN_IF_ERROR(
- row->SetBinaryNoCopy(col, slice), VALUE_ERROR_MSG);
- }
- } else {
- if (copy_strings) {
- KUDU_RETURN_IF_ERROR(
- row->SetString(col, slice), VALUE_ERROR_MSG);
- } else {
- KUDU_RETURN_IF_ERROR(
- row->SetStringNoCopy(col, slice), VALUE_ERROR_MSG);
- }
+ switch (col_type.StringSubtype()) {
+ case StringValSubtype::BINARY:
+ if (copy_strings) {
+ KUDU_RETURN_IF_ERROR(
+ row->SetBinary(col, slice), VALUE_ERROR_MSG);
+ } else {
+ KUDU_RETURN_IF_ERROR(
+ row->SetBinaryNoCopy(col, slice), VALUE_ERROR_MSG);
+ }
+ break;
+ case StringValSubtype::STRING:
+ if (copy_strings) {
+ KUDU_RETURN_IF_ERROR(
+ row->SetString(col, slice), VALUE_ERROR_MSG);
+ } else {
+ KUDU_RETURN_IF_ERROR(
+ row->SetStringNoCopy(col, slice), VALUE_ERROR_MSG);
+ }
+ break;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << col_type.StringSubtype();
}
break;
}
diff --git a/be/src/exec/paimon/paimon-jni-row-reader.cc b/be/src/exec/paimon/paimon-jni-row-reader.cc
index 00990c537..29a3d767b 100644
--- a/be/src/exec/paimon/paimon-jni-row-reader.cc
+++ b/be/src/exec/paimon/paimon-jni-row-reader.cc
@@ -95,12 +95,17 @@ Status PaimonJniRowReader::WriteSlot(const arrow::Array* array, int row_index,
}
case TYPE_STRING:
case TYPE_VARCHAR: {
- if (type.IsBinaryType()) { // byte[]
- RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ true>(
- array, row_index, slot, tuple_data_pool));
- } else {
- RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ false>(
- array, row_index, slot, tuple_data_pool));
+ switch (type.StringSubtype()) {
+ case StringValSubtype::BINARY: // byte[]
+ RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ true>(
+ array, row_index, slot, tuple_data_pool));
+ break;
+ case StringValSubtype::STRING:
+ RETURN_IF_ERROR(WriteStringOrBinarySlot</* IS_BINARY */ false>(
+ array, row_index, slot, tuple_data_pool));
+ break;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << type.StringSubtype();
}
break;
}
diff --git a/be/src/exec/text-converter.inline.h b/be/src/exec/text-converter.inline.h
index 15df3e349..6cf2a8be2 100644
--- a/be/src/exec/text-converter.inline.h
+++ b/be/src/exec/text-converter.inline.h
@@ -67,6 +67,7 @@ inline bool TextConverter::WriteSlot(const SlotDescriptor* slot_desc, Tuple* tup
!(len != 0 && (copy_string || need_escape));
bool base64_decode = false;
+ DCHECK(type.IsPlainStringType() || type.IsBinaryType());
if (type.IsBinaryType() && decode_binary_ && len != 0) {
base64_decode = true;
reuse_data = false;
diff --git a/be/src/runtime/complex-value-writer.inline.h b/be/src/runtime/complex-value-writer.inline.h
index e22dc6bf1..09462a8df 100644
--- a/be/src/runtime/complex-value-writer.inline.h
+++ b/be/src/runtime/complex-value-writer.inline.h
@@ -59,6 +59,7 @@ void ComplexValueWriter<JsonStream>::PrimitiveValueToJSON(void* value,
RawValue::PrintValue(value, type, scale, &tmp);
const bool should_convert_to_string = map_key && stringify_map_keys_;
if (IsPrimitiveTypePrintedAsString(type) || should_convert_to_string) {
+ DCHECK(type.IsPlainStringType() || type.IsBinaryType());
if (type.IsBinaryType()) {
int64_t base64_max_len;
bool succ = Base64EncodeBufLen(tmp.size(), &base64_max_len);
diff --git a/be/src/runtime/types.cc b/be/src/runtime/types.cc
index 535ce97fc..50aa1373f 100644
--- a/be/src/runtime/types.cc
+++ b/be/src/runtime/types.cc
@@ -38,7 +38,8 @@ const int ColumnType::MAX_DECIMAL8_PRECISION;
const char* ColumnType::LLVM_CLASS_NAME = "struct.impala::ColumnType";
ColumnType::ColumnType(const std::vector<TTypeNode>& types, int* idx)
- : len(-1), precision(-1), scale(-1), is_binary_(false) {
+ : len(-1), precision(-1), scale(-1),
+ string_val_subtype_(StringValSubtype::STRING) {
DCHECK_GE(*idx, 0);
DCHECK_LT(*idx, types.size());
const TTypeNode& node = types[*idx];
@@ -52,7 +53,9 @@ ColumnType::ColumnType(const std::vector<TTypeNode>& types, int* idx)
DCHECK(scalar_type.__isset.len);
len = scalar_type.len;
} else if (type == TYPE_STRING) {
- is_binary_ = scalar_type.type == TPrimitiveType::BINARY;
+ if (scalar_type.type == TPrimitiveType::BINARY) {
+ string_val_subtype_ = StringValSubtype::BINARY;
+ }
} else if (type == TYPE_DECIMAL) {
DCHECK(scalar_type.__isset.precision);
DCHECK(scalar_type.__isset.scale);
@@ -116,7 +119,7 @@ PrimitiveType ThriftToType(TPrimitiveType::type ttype) {
}
}
-TPrimitiveType::type ToThrift(PrimitiveType ptype, bool is_binary) {
+TPrimitiveType::type ToThrift(PrimitiveType ptype, StringValSubtype subtype) {
switch (ptype) {
case INVALID_TYPE: return TPrimitiveType::INVALID_TYPE;
case TYPE_NULL: return TPrimitiveType::NULL_TYPE;
@@ -131,7 +134,13 @@ TPrimitiveType::type ToThrift(PrimitiveType ptype, bool is_binary) {
case TYPE_DATETIME: return TPrimitiveType::DATETIME;
case TYPE_TIMESTAMP: return TPrimitiveType::TIMESTAMP;
case TYPE_STRING:
- return is_binary ? TPrimitiveType::BINARY : TPrimitiveType::STRING;
+ switch (subtype) {
+ case StringValSubtype::BINARY: return TPrimitiveType::BINARY;
+ case StringValSubtype::STRING: return TPrimitiveType::STRING;
+ default:
+ DCHECK(false) << "Unexpected string subtype" << subtype;
+ return TPrimitiveType::INVALID_TYPE;
+ }
case TYPE_VARCHAR: return TPrimitiveType::VARCHAR;
case TYPE_BINARY:
DCHECK(false) << "STRING should be used instead of BINARY in the backend.";
@@ -243,7 +252,7 @@ void ColumnType::ToThrift(TColumnType* thrift_type) const {
node.type = TTypeNodeType::SCALAR;
node.__set_scalar_type(TScalarType());
TScalarType& scalar_type = node.scalar_type;
- scalar_type.__set_type(impala::ToThrift(type, is_binary_));
+ scalar_type.__set_type(impala::ToThrift(type, string_val_subtype_));
if (type == TYPE_CHAR || type == TYPE_VARCHAR
|| type == TYPE_FIXED_UDA_INTERMEDIATE) {
DCHECK_NE(len, -1);
@@ -260,7 +269,13 @@ void ColumnType::ToThrift(TColumnType* thrift_type) const {
string ColumnType::DebugString() const {
switch (type) {
case TYPE_STRING:
- return is_binary_ ? "BINARY" : "STRING";
+ switch (string_val_subtype_) {
+ case StringValSubtype::BINARY: return "BINARY";
+ case StringValSubtype::STRING: return "STRING";
+ default:
+ DCHECK(false) << "Unexpected string subtype" << string_val_subtype_;
+ return "";
+ }
case TYPE_CHAR:
return Substitute("CHAR($0)", len);
case TYPE_DECIMAL:
@@ -286,6 +301,15 @@ ostream& operator<<(ostream& os, const ColumnType& type) {
return os;
}
+ostream& operator<<(ostream& os, StringValSubtype subtype) {
+ switch (subtype) {
+ case StringValSubtype::STRING: os << "STRING"; break;
+ case StringValSubtype::BINARY: os << "BINARY"; break;
+ default: os << "UNKNOWN(" << static_cast<int>(subtype) << ")"; break;
+ }
+ return os;
+}
+
llvm::ConstantStruct* ColumnType::ToIR(LlvmCodeGen* codegen) const {
// ColumnType = { i32, i8, i32, i32, i32, <vector>, <vector>, <vector> }
llvm::StructType* column_type_type = codegen->GetStructType<ColumnType>();
@@ -311,8 +335,9 @@ llvm::ConstantStruct* ColumnType::ToIR(LlvmCodeGen* codegen) const {
llvm::Constant* field_ids_field =
llvm::Constant::getNullValue(column_type_type->getElementType(6));
- DCHECK_EQ(sizeof(is_binary_), sizeof(uint8_t));
- llvm::Constant* is_binary_field = codegen->GetI8Constant(is_binary_);
+ DCHECK_EQ(sizeof(string_val_subtype_), sizeof(uint8_t));
+ llvm::Constant* string_val_subtype_field =
+ codegen->GetI8Constant(static_cast<uint8_t>(string_val_subtype_));
llvm::Constant* padding =
llvm::Constant::getNullValue(column_type_type->getElementType(8));
@@ -320,7 +345,7 @@ llvm::ConstantStruct* ColumnType::ToIR(LlvmCodeGen* codegen) const {
return llvm::cast<llvm::ConstantStruct>(
llvm::ConstantStruct::get(column_type_type, type_field, len_field, precision_field,
scale_field, children_field, field_names_field, field_ids_field,
- is_binary_field, padding));
+ string_val_subtype_field, padding));
}
}
diff --git a/be/src/runtime/types.h b/be/src/runtime/types.h
index 578dfa819..6aeeb7f4c 100644
--- a/be/src/runtime/types.h
+++ b/be/src/runtime/types.h
@@ -47,7 +47,7 @@ enum PrimitiveType {
TYPE_STRING,
TYPE_DATE,
TYPE_DATETIME, // Not implemented
- TYPE_BINARY, // Not used, see ColumnType::is_binary
+ TYPE_BINARY, // Not used, see ColumnType::string_val_subtype_
TYPE_DECIMAL,
TYPE_CHAR,
TYPE_VARCHAR,
@@ -59,7 +59,13 @@ enum PrimitiveType {
};
PrimitiveType ThriftToType(TPrimitiveType::type ttype);
-TPrimitiveType::type ToThrift(PrimitiveType ptype);
+
+// Subtype of a TYPE_STRING byte array. BINARY is represented as TYPE_STRING in
+// the backend; this field distinguishes it from a plain STRING.
+enum class StringValSubtype : uint8_t { STRING, BINARY };
+
+TPrimitiveType::type ToThrift(PrimitiveType ptype,
+ StringValSubtype subtype = StringValSubtype::STRING);
std::string TypeToString(PrimitiveType t);
std::string TypeToOdbcString(const TColumnType& type);
@@ -99,7 +105,8 @@ struct ColumnType {
static const char* LLVM_CLASS_NAME;
explicit ColumnType(PrimitiveType type = INVALID_TYPE)
- : type(type), len(-1), precision(-1), scale(-1), is_binary_(false) {
+ : type(type), len(-1), precision(-1), scale(-1),
+ string_val_subtype_(StringValSubtype::STRING) {
DCHECK_NE(type, TYPE_CHAR);
DCHECK_NE(type, TYPE_VARCHAR);
DCHECK_NE(type, TYPE_BINARY);
@@ -138,7 +145,7 @@ struct ColumnType {
static ColumnType CreateBinaryType() {
ColumnType ret(TYPE_STRING);
- ret.is_binary_ = true;
+ ret.string_val_subtype_ = StringValSubtype::BINARY;
return ret;
}
@@ -181,7 +188,7 @@ struct ColumnType {
if (children != o.children) return false;
if (type == TYPE_CHAR || type == TYPE_FIXED_UDA_INTERMEDIATE) return len == o.len;
if (type == TYPE_DECIMAL) return precision == o.precision && scale == o.scale;
- if (type == TYPE_STRING) return is_binary_ == o.is_binary_;
+ if (type == TYPE_STRING) return string_val_subtype_ == o.string_val_subtype_;
return true;
}
@@ -220,7 +227,17 @@ struct ColumnType {
return type == TYPE_STRING || type == TYPE_VARCHAR;
}
- inline bool IsBinaryType() const { return is_binary_; }
+ inline bool IsBinaryType() const {
+ return string_val_subtype_ == StringValSubtype::BINARY;
+ }
+
+ // True unless this is the BINARY subtype, i.e. a plain STRING value
+ // (or any non-TYPE_STRING type, whose subtype defaults to STRING).
+ inline bool IsPlainStringType() const {
+ return string_val_subtype_ == StringValSubtype::STRING;
+ }
+
+ inline StringValSubtype StringSubtype() const { return string_val_subtype_; }
inline bool IsComplexType() const {
return type == TYPE_STRUCT || type == TYPE_ARRAY || type == TYPE_MAP;
@@ -269,10 +286,7 @@ struct ColumnType {
// some code parts, e.g. file format readers/writers differentiate between the two.
// Instead of PrimitiveType::TYPE_BINARY, TYPE_STRING is used for the BINARY type to
// ensure that everything that works for STRING also works for BINARY.
- //
- // This variable is true if 'type' is TYPE_STRING and this object represents the BINARY
- // type, and false in all other cases.
- bool is_binary_ = false;
+ StringValSubtype string_val_subtype_ = StringValSubtype::STRING;
/// Recursive implementation of ToThrift() that populates 'thrift_type' with the
/// TTypeNodes for this type and its children.
@@ -348,6 +362,7 @@ struct ColumnType {
};
std::ostream& operator<<(std::ostream& os, const ColumnType& type);
+std::ostream& operator<<(std::ostream& os, StringValSubtype subtype);
}