You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/07/30 16:52:17 UTC
[GitHub] [arrow] rommelDB opened a new pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
rommelDB opened a new pull request #10843:
URL: https://github.com/apache/arrow/pull/10843
Changes:
- Added support for decimal128 and decimal256 json converted.
- Added unit tests for converting decimal128 and decimal256. (Note: This is not providing inferring support)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680131459
##########
File path: cpp/src/arrow/json/converter_test.cc
##########
@@ -96,5 +96,27 @@ TEST(ConverterTest, Timestamp) {
AssertConvert(timestamp(TimeUnit::SECOND), src, src);
}
+TEST(ConverterTest, Decimal128) {
+ std::string src = R"([
+ "02.0000000000",
+ "30.0000000000",
+ "22.0000000000",
+ "-121.0000000000",
+ null])";
+
+ AssertConvert(decimal128(38, 10), src, src);
Review comment:
The test is confusing because we use the same string representation to produce the expected one. I think that rewriting this test should be clear such confusion.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680139090
##########
File path: cpp/src/arrow/json/converter.cc
##########
@@ -147,6 +149,41 @@ class NumericConverter : public PrimitiveConverter {
const T& numeric_type_;
};
+template <typename T>
+class DecimalConverter : public PrimitiveConverter {
+ public:
+ using value_type = typename TypeTraits<T>::BuilderType::ValueType;
+
+ DecimalConverter(MemoryPool* pool, const std::shared_ptr<DataType>& type)
+ : PrimitiveConverter(pool, type) {}
+
+ Status Convert(const std::shared_ptr<Array>& in, std::shared_ptr<Array>* out) override {
+ if (in->type_id() == Type::NA) {
+ return MakeArrayOfNull(out_type_, in->length(), pool_).Value(out);
+ }
+ const auto& dict_array = GetDictionaryArray(in);
+
+ using Builder = typename TypeTraits<T>::BuilderType;
+ Builder builder(out_type_, pool_);
+ RETURN_NOT_OK(builder.Resize(dict_array.indices()->length()));
+
+ auto visit_valid = [&](string_view repr) {
+ ARROW_ASSIGN_OR_RAISE(value_type value,
+ TypeTraits<T>::BuilderType::ValueType::FromString(repr));
+ builder.UnsafeAppend(value);
+ return Status::OK();
+ };
+
+ auto visit_null = [&]() {
Review comment:
Makes sense, there we are not capturing lot of variables, so it would be clearer if I just capture the builder by reference.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#issuecomment-890023863
https://issues.apache.org/jira/browse/ARROW-4700
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipeblazing commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
felipeblazing commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680128769
##########
File path: cpp/src/arrow/json/converter.cc
##########
@@ -147,6 +149,41 @@ class NumericConverter : public PrimitiveConverter {
const T& numeric_type_;
};
+template <typename T>
+class DecimalConverter : public PrimitiveConverter {
+ public:
+ using value_type = typename TypeTraits<T>::BuilderType::ValueType;
+
+ DecimalConverter(MemoryPool* pool, const std::shared_ptr<DataType>& type)
+ : PrimitiveConverter(pool, type) {}
+
+ Status Convert(const std::shared_ptr<Array>& in, std::shared_ptr<Array>* out) override {
+ if (in->type_id() == Type::NA) {
+ return MakeArrayOfNull(out_type_, in->length(), pool_).Value(out);
+ }
+ const auto& dict_array = GetDictionaryArray(in);
+
+ using Builder = typename TypeTraits<T>::BuilderType;
+ Builder builder(out_type_, pool_);
+ RETURN_NOT_OK(builder.Resize(dict_array.indices()->length()));
+
+ auto visit_valid = [&](string_view repr) {
Review comment:
why capture everything here as well?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680140494
##########
File path: cpp/src/arrow/json/converter.cc
##########
@@ -147,6 +149,41 @@ class NumericConverter : public PrimitiveConverter {
const T& numeric_type_;
};
+template <typename T>
+class DecimalConverter : public PrimitiveConverter {
+ public:
+ using value_type = typename TypeTraits<T>::BuilderType::ValueType;
+
+ DecimalConverter(MemoryPool* pool, const std::shared_ptr<DataType>& type)
+ : PrimitiveConverter(pool, type) {}
+
+ Status Convert(const std::shared_ptr<Array>& in, std::shared_ptr<Array>* out) override {
+ if (in->type_id() == Type::NA) {
+ return MakeArrayOfNull(out_type_, in->length(), pool_).Value(out);
+ }
+ const auto& dict_array = GetDictionaryArray(in);
+
+ using Builder = typename TypeTraits<T>::BuilderType;
+ Builder builder(out_type_, pool_);
+ RETURN_NOT_OK(builder.Resize(dict_array.indices()->length()));
+
+ auto visit_valid = [&](string_view repr) {
Review comment:
Sure, as the other visitor I'll make that explicit. Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipeblazing commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
felipeblazing commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680127350
##########
File path: cpp/src/arrow/json/converter_test.cc
##########
@@ -96,5 +96,27 @@ TEST(ConverterTest, Timestamp) {
AssertConvert(timestamp(TimeUnit::SECOND), src, src);
}
+TEST(ConverterTest, Decimal128) {
+ std::string src = R"([
+ "02.0000000000",
+ "30.0000000000",
+ "22.0000000000",
+ "-121.0000000000",
+ null])";
+
+ AssertConvert(decimal128(38, 10), src, src);
Review comment:
Namely that I would expect something more like instantiating decimals of the correct precision with the values you specified there and then verifying that they are indeed the same values in the same order.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680213131
##########
File path: cpp/src/arrow/json/parser.cc
##########
@@ -102,6 +102,8 @@ Status Kind::ForType(const DataType& type, Kind::type* kind) {
Status Visit(const TimeType&) { return SetKind(Kind::kNumber); }
Status Visit(const DateType&) { return SetKind(Kind::kNumber); }
Status Visit(const BinaryType&) { return SetKind(Kind::kString); }
+ Status Visit(const LargeBinaryType&) { return SetKind(Kind::kString); }
+ Status Visit(const TimestampType&) { return SetKind(Kind::kString); }
Review comment:
Thanks for fixing this!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680131459
##########
File path: cpp/src/arrow/json/converter_test.cc
##########
@@ -96,5 +96,27 @@ TEST(ConverterTest, Timestamp) {
AssertConvert(timestamp(TimeUnit::SECOND), src, src);
}
+TEST(ConverterTest, Decimal128) {
+ std::string src = R"([
+ "02.0000000000",
+ "30.0000000000",
+ "22.0000000000",
+ "-121.0000000000",
+ null])";
+
+ AssertConvert(decimal128(38, 10), src, src);
Review comment:
The test is confusing because we use the same string representation to produce the expected one. I think that after rewriting this test should be clear such confusion.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680131459
##########
File path: cpp/src/arrow/json/converter_test.cc
##########
@@ -96,5 +96,27 @@ TEST(ConverterTest, Timestamp) {
AssertConvert(timestamp(TimeUnit::SECOND), src, src);
}
+TEST(ConverterTest, Decimal128) {
+ std::string src = R"([
+ "02.0000000000",
+ "30.0000000000",
+ "22.0000000000",
+ "-121.0000000000",
+ null])";
+
+ AssertConvert(decimal128(38, 10), src, src);
Review comment:
The test is confusing because we use the same string representation to produce the expected one. I think that after rewriting this test should clarify such confusion.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB edited a comment on pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB edited a comment on pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#issuecomment-890024085
Please, take a look at it @bkietz
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10843:
URL: https://github.com/apache/arrow/pull/10843
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipeblazing commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
felipeblazing commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680124458
##########
File path: cpp/src/arrow/json/converter_test.cc
##########
@@ -96,5 +96,27 @@ TEST(ConverterTest, Timestamp) {
AssertConvert(timestamp(TimeUnit::SECOND), src, src);
}
+TEST(ConverterTest, Decimal128) {
+ std::string src = R"([
+ "02.0000000000",
+ "30.0000000000",
+ "22.0000000000",
+ "-121.0000000000",
+ null])";
+
+ AssertConvert(decimal128(38, 10), src, src);
Review comment:
How does this verify that the value was converted to the correct value if we aren't passing in a decimal128 with the correct value specified?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680237364
##########
File path: cpp/src/arrow/json/converter_test.cc
##########
@@ -96,5 +96,27 @@ TEST(ConverterTest, Timestamp) {
AssertConvert(timestamp(TimeUnit::SECOND), src, src);
}
+TEST(ConverterTest, Decimal128) {
+ std::string src = R"([
+ "02.0000000000",
+ "30.0000000000",
+ "22.0000000000",
+ "-121.0000000000",
+ null])";
+
+ AssertConvert(decimal128(38, 10), src, src);
Review comment:
@felipeblazing I just did a refactoring of the tests, and now I think the intention is much clearer. So now, the expected and input values are separate instances, both in JSON format. Namely, the first is an array from JSON that holds the expected values, and the second one is a one object per line JSON table as the input string (thank you @bkietz for your support 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rommelDB commented on pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
rommelDB commented on pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#issuecomment-890024085
Please take it a look @bkietz
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipeblazing commented on a change in pull request #10843: ARROW-4700: [C++] Added support for decimal128 and decimal256 json converted
Posted by GitBox <gi...@apache.org>.
felipeblazing commented on a change in pull request #10843:
URL: https://github.com/apache/arrow/pull/10843#discussion_r680128199
##########
File path: cpp/src/arrow/json/converter.cc
##########
@@ -147,6 +149,41 @@ class NumericConverter : public PrimitiveConverter {
const T& numeric_type_;
};
+template <typename T>
+class DecimalConverter : public PrimitiveConverter {
+ public:
+ using value_type = typename TypeTraits<T>::BuilderType::ValueType;
+
+ DecimalConverter(MemoryPool* pool, const std::shared_ptr<DataType>& type)
+ : PrimitiveConverter(pool, type) {}
+
+ Status Convert(const std::shared_ptr<Array>& in, std::shared_ptr<Array>* out) override {
+ if (in->type_id() == Type::NA) {
+ return MakeArrayOfNull(out_type_, in->length(), pool_).Value(out);
+ }
+ const auto& dict_array = GetDictionaryArray(in);
+
+ using Builder = typename TypeTraits<T>::BuilderType;
+ Builder builder(out_type_, pool_);
+ RETURN_NOT_OK(builder.Resize(dict_array.indices()->length()));
+
+ auto visit_valid = [&](string_view repr) {
+ ARROW_ASSIGN_OR_RAISE(value_type value,
+ TypeTraits<T>::BuilderType::ValueType::FromString(repr));
+ builder.UnsafeAppend(value);
+ return Status::OK();
+ };
+
+ auto visit_null = [&]() {
Review comment:
Do we really want to capture everything by reference here and instead just pass in what we know we need?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org