You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/11/01 08:30:57 UTC
[GitHub] [ignite-3] SammyVimes opened a new pull request, #1289: IGNITE-18017 Add C++ tests for binary tuple
SammyVimes opened a new pull request, #1289:
URL: https://github.com/apache/ignite-3/pull/1289
https://issues.apache.org/jira/browse/IGNITE-18017
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1011800063
##########
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 4 /* scale */ + value.get_unscaled_value().byte_size();
Review Comment:
This violates Binary Tuple Format. We don't store decimal scale in binary tuple. It is stored in schema.
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012560317
##########
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:
Sorry, it's a leftover from the incorrect calculation
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes merged pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes merged PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012559168
##########
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:
Only in a currently muted test
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012695435
##########
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:
Added an append
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012620679
##########
modules/platforms/cpp/ignite/schema/binary_tuple_builder.h:
##########
@@ -212,88 +268,282 @@ class binary_tuple_builder {
/**
* @brief Appends a value for the next element.
*
+ * @param type Element type.
+ * @param value Value of an internal tuple field.
+ */
+ void append(ignite_type type, const bytes_view &bytes);
+
+ /**
+ * @brief Appends a value or null for the next element.
+ *
+ * @tparam BytesT Byte container for a single internal tuple field.
+ * @param type Element type.
+ * @param slice Optional value of an internal tuple field.
+ */
+ template <typename BytesT>
+ void append(ignite_type type, const std::optional<BytesT> &slice) {
+ if (slice.has_value()) {
+ append(type, slice.value());
+ } else {
+ append(std::nullopt);
+ }
+ }
+
+ /**
+ * @brief Appends values for a number of elements.
+ *
+ * @tparam BytesT Byte container for a single internal tuple field.
+ * @param schema Tuple schema.
+ * @param tuple Tuple in the internal form.
+ */
+ template <typename BytesT>
+ void append(const binary_tuple_schema &schema, const std::vector<std::optional<BytesT>> &tuple) {
+ for (IntT i = 0; i < schema.num_elements(); i++) {
+ append(schema.get_element(i).dataType, tuple[i]);
+ }
+ }
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_bytes(bytes_view bytes);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_int8(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
* @param value Element value.
*/
- void append_int8(std::int8_t value) { put_int8(value); }
+ void append_int8(std::int8_t value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_int16(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
*
* @param value Element value.
*/
- void append_int16(std::int16_t value) { put_int16(value); }
+ void append_int16(std::int16_t value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_int32(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
*
* @param value Element value.
*/
- void append_int32(std::int32_t value) { put_int32(value); }
+ void append_int32(std::int32_t value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_int64(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
*
* @param value Element value.
*/
- void append_int64(std::int64_t value) { put_int64(value); }
+ void append_int64(std::int64_t value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_float(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
*
* @param value Element value.
*/
- void append_float(float value) { put_float(value); }
+ void append_float(float value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_double(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
*
* @param value Element value.
*/
- void append_double(double value) { put_double(value); }
+ void append_double(double value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_number(bytes_view bytes);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param value Big integer value.
+ */
+ void append_number(const big_integer &value);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param value Big decimal value.
+ */
+ void append_number(const big_decimal &value);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_uuid(bytes_view bytes);
+
+ /**
+ * @brief Writes specified element.
+ *
+ * The written value may differ from the original because of value compression.
*
* @param value Element value.
*/
- void append_uuid(uuid value) { put_uuid(value); }
+ void append_uuid(uuid value);
/**
- * @brief Appends a value for the next element.
+ * @brief Writes binary value of specified element.
*
- * @param type Element type.
- * @param value Value of an internal tuple field.
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
*/
- void append(ignite_type type, const bytes_view &bytes);
+ void append_date(bytes_view bytes);
/**
- * @brief Appends a value or null for the next element.
+ * @brief Writes binary value of specified element.
*
- * @tparam BytesT Byte container for a single internal tuple field.
- * @param type Element type.
- * @param slice Optional value of an internal tuple field.
+ * The written value may differ from the original because of value compression.
+ *
+ * @param value Date value.
*/
- template <typename BytesT>
- void append(ignite_type type, const std::optional<BytesT> &slice) {
- if (slice.has_value()) {
- append(type, slice.value());
- } else {
- append(std::nullopt);
- }
- }
+ void append_date(const ignite_date &value);
/**
- * @brief Appends values for a number of elements.
+ * @brief Writes binary value of specified element.
*
- * @tparam BytesT Byte container for a single internal tuple field.
- * @param schema Tuple schema.
- * @param tuple Tuple in the internal form.
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
*/
- template <typename BytesT>
- void append(const binary_tuple_schema &schema, const std::vector<std::optional<BytesT>> &tuple) {
- for (IntT i = 0; i < schema.num_elements(); i++) {
- append(schema.get_element(i).dataType, tuple[i]);
- }
+ void append_time(bytes_view bytes);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param value Time value.
+ */
+ void append_time(const ignite_time &value);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_date_time(bytes_view bytes);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param value Date time value.
+ */
+ void append_date_time(const ignite_date_time &value);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param bytes Binary element value.
+ */
+ void append_timestamp(bytes_view bytes);
+
+ /**
+ * @brief Writes binary value of specified element.
+ *
+ * The written value may differ from the original because of value compression.
+ *
+ * @param value Timestamp value.
+ */
+ void append_timestamp(const ignite_timestamp &value);
+
+ /**
+ * @brief Appends a string for the next element.
+ *
+ * @param value Element value.
+ */
+ void append_string(const std::string &value) {
+ ignite::bytes_view bytes{reinterpret_cast<const std::byte *>(value.data()), value.size()};
Review Comment:
Again, why do you use the ignite prefix here?
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012582899
##########
modules/platforms/cpp/ignite/schema/tuple_assembler.h:
##########
@@ -145,3 +142,4 @@ class tuple_assembler {
return builder.build();
}
};
+}
Review Comment:
`} // ignite`
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1011802549
##########
modules/platforms/cpp/ignite/schema/binary_tuple_parser.cpp:
##########
@@ -251,6 +252,12 @@ 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) {
+ auto scale = load_as<std::int32_t>(bytes);
Review Comment:
The scale should be provided as A method parameter. It is not stored in binary tuple. It is defined in schema.
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012579902
##########
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:
Got it
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012290339
##########
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 4 /* scale */ + value.get_unscaled_value().byte_size();
Review Comment:
Right, fixed
##########
modules/platforms/cpp/ignite/schema/binary_tuple_parser.cpp:
##########
@@ -251,6 +252,12 @@ 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) {
+ auto scale = load_as<std::int32_t>(bytes);
Review Comment:
Fixed
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012580033
##########
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:
Done
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012603498
##########
modules/platforms/cpp/ignite/schema/tuple_test.cpp:
##########
@@ -0,0 +1,1268 @@
+/*
+ * 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 "tuple_assembler.h"
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <ignite/schema/binary_tuple_parser.h>
+#include <ignite/schema/column_info.h>
Review Comment:
Fixed
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012595047
##########
modules/platforms/cpp/ignite/schema/tuple_test.cpp:
##########
@@ -0,0 +1,1268 @@
+/*
+ * 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 "tuple_assembler.h"
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <ignite/schema/binary_tuple_parser.h>
+#include <ignite/schema/column_info.h>
Review Comment:
The include order does not match our conventions.
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012624531
##########
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:
It's not clear how it is going to work now. It does `claim` but there is no corresponding `append`.
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012624531
##########
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:
It's not clear how it is going to work now.
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ademakov commented on a diff in pull request #1289: IGNITE-18017 Add C++ tests for binary tuple
Posted by GitBox <gi...@apache.org>.
ademakov commented on code in PR #1289:
URL: https://github.com/apache/ignite-3/pull/1289#discussion_r1012582899
##########
modules/platforms/cpp/ignite/schema/tuple_assembler.h:
##########
@@ -145,3 +142,4 @@ class tuple_assembler {
return builder.build();
}
};
+}
Review Comment:
`} // namespace ignite`
--
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@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org