You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2023/04/03 05:25:16 UTC
[GitHub] [orc] wgtmac commented on a diff in pull request #1454: ORC-1385: [C++] Support schema evolution of numeric types
wgtmac commented on code in PR #1454:
URL: https://github.com/apache/orc/pull/1454#discussion_r1155482409
##########
c++/include/orc/Vector.hh:
##########
@@ -53,6 +53,10 @@ namespace orc {
// whether the vector batch is encoded
bool isEncoded;
+ // whether the vector is not a numeric vector
+ // or it's created with the option useTightNumericVector
+ bool isTight = true;
Review Comment:
Is it possible not to add this? The reader can get this information by the provided `ReaderOption`
##########
c++/include/orc/Reader.hh:
##########
@@ -340,12 +340,22 @@ namespace orc {
/**
* Set read type for schema evolution
*/
- RowReaderOptions& setReadType(std::shared_ptr<Type>& type);
+ RowReaderOptions& setReadType(std::shared_ptr<Type> type);
/**
* Get read type for schema evolution
*/
std::shared_ptr<Type>& getReadType() const;
+
+ /**
+ * Set should reader throw a exception on conversion between different data types overflow
Review Comment:
```suggestion
* Set whether reader throws or returns null when value overflows for schema evolution.
```
##########
c++/src/ConvertColumnReader.cc:
##########
@@ -0,0 +1,432 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "ConvertColumnReader.hh"
+
+namespace orc {
+
+ // Assume that we are using tight numeric vector batch
+ using BooleanVectorBatch = ByteVectorBatch;
+
+ ConvertColumnReader::ConvertColumnReader(const Type& _readType, const Type& fileType,
+ StripeStreams& stripe, bool _throwOnOverflow)
+ : ColumnReader(_readType, stripe), readType(_readType), throwOnOverflow(_throwOnOverflow) {
+ reader = buildReader(fileType, stripe, /*useTightNumericVector=*/true,
+ /*throwOnOverflow=*/false, /*convertToReadType*/ false);
+ data =
+ fileType.createRowBatch(0, memoryPool, /*encoded=*/false, /*useTightNumericVector=*/true);
+ }
+
+ void ConvertColumnReader::next(ColumnVectorBatch& rowBatch, uint64_t numValues, char* notNull) {
+ if (!rowBatch.isTight) {
+ throw SchemaEvolutionError(
+ "SchemaEvolution only support tight vector, please create ColumnVectorBatch with option "
+ "useTightNumericVector");
+ }
+ reader->next(*data, numValues, notNull);
+ rowBatch.resize(data->capacity);
+ rowBatch.numElements = data->numElements;
+ rowBatch.hasNulls = data->hasNulls;
+ if (!rowBatch.hasNulls) {
+ memset(rowBatch.notNull.data(), 1, data->notNull.size());
Review Comment:
I guess this is not required
##########
c++/src/ConvertColumnReader.cc:
##########
@@ -0,0 +1,432 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "ConvertColumnReader.hh"
+
+namespace orc {
+
+ // Assume that we are using tight numeric vector batch
+ using BooleanVectorBatch = ByteVectorBatch;
+
+ ConvertColumnReader::ConvertColumnReader(const Type& _readType, const Type& fileType,
+ StripeStreams& stripe, bool _throwOnOverflow)
+ : ColumnReader(_readType, stripe), readType(_readType), throwOnOverflow(_throwOnOverflow) {
+ reader = buildReader(fileType, stripe, /*useTightNumericVector=*/true,
+ /*throwOnOverflow=*/false, /*convertToReadType*/ false);
+ data =
+ fileType.createRowBatch(0, memoryPool, /*encoded=*/false, /*useTightNumericVector=*/true);
+ }
+
+ void ConvertColumnReader::next(ColumnVectorBatch& rowBatch, uint64_t numValues, char* notNull) {
+ if (!rowBatch.isTight) {
+ throw SchemaEvolutionError(
+ "SchemaEvolution only support tight vector, please create ColumnVectorBatch with option "
+ "useTightNumericVector");
+ }
+ reader->next(*data, numValues, notNull);
+ rowBatch.resize(data->capacity);
+ rowBatch.numElements = data->numElements;
+ rowBatch.hasNulls = data->hasNulls;
+ if (!rowBatch.hasNulls) {
+ memset(rowBatch.notNull.data(), 1, data->notNull.size());
+ } else {
+ memcpy(rowBatch.notNull.data(), data->notNull.data(), data->notNull.size());
+ }
+ }
+
+ uint64_t ConvertColumnReader::skip(uint64_t numValues) {
+ return reader->skip(numValues);
+ }
+
+ void ConvertColumnReader::seekToRowGroup(
+ std::unordered_map<uint64_t, PositionProvider>& positions) {
+ reader->seekToRowGroup(positions);
+ }
+
+ static inline bool canFitInLong(double value) {
+ constexpr double MIN_LONG_AS_DOUBLE = -0x1p63;
+ constexpr double MAX_LONG_AS_DOUBLE_PLUS_ONE = 0x1p63;
+ return ((MIN_LONG_AS_DOUBLE - value < 1.0) && (value < MAX_LONG_AS_DOUBLE_PLUS_ONE));
+ }
+
+ template <typename FileType, typename ReadType>
+ static inline void handleOverflow(ColumnVectorBatch& dstBatch, uint64_t idx, bool shouldThrow) {
+ if (!shouldThrow) {
+ dstBatch.notNull.data()[idx] = 0;
+ dstBatch.hasNulls = true;
+ } else {
+ std::ostringstream ss;
+ ss << "Overflow when convert from " << typeid(FileType).name() << " to "
+ << typeid(ReadType).name();
+ throw SchemaEvolutionError(ss.str());
+ }
+ }
+
+ // return false if overflow
+ template <typename ReadType>
+ static bool downCastToInteger(ReadType& dstValue, int64_t inputLong) {
+ dstValue = static_cast<ReadType>(inputLong);
+ if constexpr (std::is_same<ReadType, int64_t>::value) {
+ return true;
+ }
+ if (static_cast<int64_t>(dstValue) != inputLong) {
+ return false;
+ }
+ return true;
+ }
+
+ // set null or throw exception if overflow
+ template <typename ReadType, typename FileType>
+ static inline void convertNumericElement(const FileType& srcValue, ReadType& destValue,
+ ColumnVectorBatch& destBatch, uint64_t idx,
+ bool shouldThrow) {
+ constexpr bool isFileTypeFloatingPoint(std::is_floating_point<FileType>::value);
+ constexpr bool isReadTypeFloatingPoint(std::is_floating_point<ReadType>::value);
+ int64_t longValue = static_cast<int64_t>(srcValue);
+ if (isFileTypeFloatingPoint) {
+ if (isReadTypeFloatingPoint) {
+ destValue = static_cast<ReadType>(srcValue);
+ } else {
+ if (!canFitInLong(static_cast<double>(srcValue)) ||
+ !downCastToInteger(destValue, longValue)) {
+ handleOverflow<FileType, ReadType>(destBatch, idx, shouldThrow);
+ return;
Review Comment:
This line is redundant.
##########
c++/src/ConvertColumnReader.cc:
##########
@@ -0,0 +1,432 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "ConvertColumnReader.hh"
+
+namespace orc {
+
+ // Assume that we are using tight numeric vector batch
+ using BooleanVectorBatch = ByteVectorBatch;
+
+ ConvertColumnReader::ConvertColumnReader(const Type& _readType, const Type& fileType,
+ StripeStreams& stripe, bool _throwOnOverflow)
+ : ColumnReader(_readType, stripe), readType(_readType), throwOnOverflow(_throwOnOverflow) {
+ reader = buildReader(fileType, stripe, /*useTightNumericVector=*/true,
Review Comment:
What about add an input parameter `useTightNumericVector` to the function and throw if it is false? In this way, we don't have to check it any more like line 36 does.
##########
c++/src/ColumnReader.cc:
##########
@@ -1699,7 +1712,21 @@ namespace orc {
* Create a reader for the given stripe.
*/
std::unique_ptr<ColumnReader> buildReader(const Type& type, StripeStreams& stripe,
- bool useTightNumericVector) {
+ bool useTightNumericVector,
+ bool throwOnSchemaEvolutionOverflow,
+ bool convertToReadType) {
+ if (convertToReadType && stripe.getSchemaEvolution()) {
+ if (stripe.getSchemaEvolution()->needConvert(type)) {
+ if (!useTightNumericVector) {
+ throw SchemaEvolutionError(
+ "SchemaEvolution only support tight vector, please create ColumnVectorBatch with "
+ "option "
+ "useTightNumericVector");
Review Comment:
```suggestion
"SchemaEvolution only support tight vector, please create ColumnVectorBatch with "
"option useTightNumericVector");
```
##########
c++/include/orc/Reader.hh:
##########
@@ -340,12 +340,22 @@ namespace orc {
/**
* Set read type for schema evolution
*/
- RowReaderOptions& setReadType(std::shared_ptr<Type>& type);
+ RowReaderOptions& setReadType(std::shared_ptr<Type> type);
/**
* Get read type for schema evolution
*/
std::shared_ptr<Type>& getReadType() const;
+
+ /**
+ * Set should reader throw a exception on conversion between different data types overflow
+ */
+ RowReaderOptions& throwOnSchemaEvolutionOverflow(bool shouldThrow);
+
+ /**
+ * Get should reader throw a exception on conversion between different data types overflow
Review Comment:
```suggestion
* Whether reader throws or returns null when value overflows for schema evolution.
```
##########
c++/src/ColumnReader.cc:
##########
@@ -1699,7 +1712,21 @@ namespace orc {
* Create a reader for the given stripe.
*/
std::unique_ptr<ColumnReader> buildReader(const Type& type, StripeStreams& stripe,
- bool useTightNumericVector) {
+ bool useTightNumericVector,
+ bool throwOnSchemaEvolutionOverflow,
+ bool convertToReadType) {
+ if (convertToReadType && stripe.getSchemaEvolution()) {
Review Comment:
Can we merge these if statements to a single one?
##########
c++/src/Reader.cc:
##########
@@ -1205,8 +1215,32 @@ namespace orc {
}
std::unique_ptr<ColumnVectorBatch> RowReaderImpl::createRowBatch(uint64_t capacity) const {
- return getSelectedType().createRowBatch(capacity, *contents->pool, enableEncodedBlock,
- useTightNumericVector);
+ if (schemaEvolution.getReadType() && selectedSchema.get() == nullptr) {
Review Comment:
nit: use `RowReaderImpl::getSelectedType()` to get selectedSchema so we can make sure it always exists.
##########
c++/src/Reader.cc:
##########
@@ -516,6 +518,13 @@ namespace orc {
return forcedScaleOnHive11Decimal;
}
+ void RowReaderImpl::getReadColumns(const Type* readType, std::set<uint64_t>& readColumns) const {
Review Comment:
nit: `getReadColumns` can be a non-member function.
##########
c++/src/Reader.cc:
##########
@@ -1205,8 +1215,32 @@ namespace orc {
}
std::unique_ptr<ColumnVectorBatch> RowReaderImpl::createRowBatch(uint64_t capacity) const {
- return getSelectedType().createRowBatch(capacity, *contents->pool, enableEncodedBlock,
- useTightNumericVector);
+ if (schemaEvolution.getReadType() && selectedSchema.get() == nullptr) {
+ auto fileSchema = &getSelectedType();
+ auto readType = schemaEvolution.getReadType();
+ std::set<uint64_t> readColumns;
+ getReadColumns(readType, readColumns);
Review Comment:
What about this?
```cpp
getReadColumns(readType, readColumnIds);
getReadColumns(getSelectedType(), selectedColumnIds);
if (readColumnIds != selectedColumnIds) {
...
}
```
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org