ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012529448
##########
modules/platforms/cpp/ignite/schema/binary_tuple_parser.cpp:
##########
@@ -251,6 +252,11 @@ big_integer binary_tuple_parser::get_number(bytes_view
bytes) {
return big_integer(bytes.data(), bytes.size());
}
+big_decimal binary_tuple_parser::get_decimal(bytes_view bytes, int32_t scale) {
+ big_integer mag(bytes.data(), bytes.size());
+ return {mag, scale};
Review Comment:
`std::move(mag)`
##########
modules/platforms/cpp/ignite/schema/binary_tuple_builder.h:
##########
@@ -328,18 +446,21 @@ class binary_tuple_builder {
/**
* @brief Checks if a value of a given integer type can be compressed to a
smaller integer type.
*
- * @tparam T Source integer type.
- * @tparam U Target integer type.
+ * @tparam SRC Source integer type.
+ * @tparam TGT Target integer type.
* @param value Source value.
* @return true If the source value can be compressed.
* @return false If the source value cannot be compressed.
*/
- template <typename T, typename U>
- static bool fits(T value) noexcept {
- static_assert(std::is_signed_v<T>);
- static_assert(std::is_signed_v<U>);
- using V = std::make_unsigned_t<U>;
- return (std::make_unsigned_t<T>(value) + std::numeric_limits<U>::max()
+ 1) <= std::numeric_limits<V>::max();
+ template <typename SRC, typename TGT>
+ static bool fits(SRC value) noexcept {
+ static_assert(std::is_signed_v<SRC>);
+ static_assert(std::is_signed_v<TGT>);
+ using U_TGT = std::make_unsigned_t<TGT>;
+
+ // Check if TGT::min <= value <= TGT::max.
+ return std::make_unsigned_t<SRC>(value +
std::numeric_limits<TGT>::max() + 1)
+ <= std::numeric_limits<U_TGT>::max();
Review Comment:
Did you clang-format this? Also I think we can get rid of U_TGT.
##########
modules/platforms/cpp/ignite/schema/tuple_assembler.h:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <ignite/schema/binary_tuple_schema.h>
+#include <ignite/schema/binary_tuple_builder.h>
+
+#include <cstring>
+#include <optional>
+#include <tuple>
+#include <type_traits>
+#include <vector>
+
+/**
+* This used to be the main tool to construct binary tuples (in IEP-54 format).
+* Now this is just a helper for unit tests.
+*/
+class tuple_assembler {
+private:
+ ignite::binary_tuple_schema schema;
+
+ ignite::binary_tuple_builder builder;
+
+public:
+ /** Construct a new Tuple Assembler object. */
+ explicit tuple_assembler(ignite::binary_tuple_schema &sch)
+ : schema(std::move(sch))
+ , builder(schema.num_elements()) { }
+
+ /** Start a new tuple. */
+ void start() {
+ builder.start();
+ }
+
+ /** Append a null value. */
+ void appendNull() {
Review Comment:
1. `append_null` 2. is it still used somewhere?
##########
modules/platforms/cpp/ignite/schema/binary_tuple_builder.h:
##########
@@ -258,6 +314,68 @@ class binary_tuple_builder {
*/
void append_uuid(uuid value) { put_uuid(value); }
+ /**
+ * @brief Appends a big integer for the next element.
+ *
+ * @param value Element value.
+ */
+ void append_number(const big_integer &value) { put_number(value); }
Review Comment:
Now I think there is no much point to have both `put_` and `append_` methods
essentially the same. We should lift the code from `put_` methods with
specific type to corresponding `append_` methods.
##########
modules/platforms/cpp/ignite/schema/binary_tuple_builder.h:
##########
@@ -431,7 +552,11 @@ class binary_tuple_builder {
* @return Required size.
*/
static SizeT gauge_number(const big_decimal &value) noexcept {
- return SizeT(value.is_zero() ? 0 :
value.get_unscaled_value().byte_size());
+ if (value.is_zero()) {
+ return 0;
+ }
+
+ return value.get_unscaled_value().byte_size();
Review Comment:
Hmm. What's the point of this change?
##########
modules/platforms/cpp/ignite/schema/tuple_assembler.h:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <ignite/schema/binary_tuple_schema.h>
+#include <ignite/schema/binary_tuple_builder.h>
+
+#include <cstring>
+#include <optional>
+#include <tuple>
+#include <type_traits>
+#include <vector>
+
+/**
+* This used to be the main tool to construct binary tuples (in IEP-54 format).
+* Now this is just a helper for unit tests.
+*/
+class tuple_assembler {
+private:
+ ignite::binary_tuple_schema schema;
Review Comment:
Please put the class itself into the `ignite` namespace and remove the
`ignite::` prefix elsewhere.
##########
modules/platforms/cpp/ignite/schema/binary_tuple_builder.cpp:
##########
@@ -269,9 +269,11 @@ void binary_tuple_builder::put_double(double value) {
void binary_tuple_builder::put_number(bytes_view bytes) {
big_integer value = binary_tuple_parser::get_number(bytes);
+ put_number(value);
+}
+void binary_tuple_builder::put_number(const ignite::big_integer &value) {
Review Comment:
Why use `ignite::` prefix here and below?
##########
modules/platforms/cpp/ignite/schema/tuple_assembler.h:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <ignite/schema/binary_tuple_schema.h>
+#include <ignite/schema/binary_tuple_builder.h>
+
+#include <cstring>
+#include <optional>
+#include <tuple>
+#include <type_traits>
+#include <vector>
+
+/**
+* This used to be the main tool to construct binary tuples (in IEP-54 format).
+* Now this is just a helper for unit tests.
+*/
+class tuple_assembler {
+private:
+ ignite::binary_tuple_schema schema;
+
+ ignite::binary_tuple_builder builder;
+
+public:
+ /** Construct a new Tuple Assembler object. */
+ explicit tuple_assembler(ignite::binary_tuple_schema &sch)
+ : schema(std::move(sch))
Review Comment:
You have to define the parameter either as `binary_tuple_schema &&sch` or
`binary_tuple_schema sch` for this to work properly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
