You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Platob (via GitHub)" <gi...@apache.org> on 2023/04/17 11:20:51 UTC
[GitHub] [arrow] Platob opened a new pull request, #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Platob opened a new pull request, #35183:
URL: https://github.com/apache/arrow/pull/35183
Compute from precision and scale to get the optimal byteWidth to store values instead of forcing to 16 and 32
It'is tested in Arrow.Tests.Types
--
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] wjones127 commented on a diff in pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #35183:
URL: https://github.com/apache/arrow/pull/35183#discussion_r1169291035
##########
csharp/test/Apache.Arrow.Tests/Decimal128ArrayTests.cs:
##########
@@ -41,7 +40,7 @@ public void AppendThenGetGivesNull()
var array = builder.Build();
Assert.Equal(3, array.Length);
- Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3);
+ Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3 + array.ByteWidth - 1);
Review Comment:
What is the purpose of this PR? It seems like it makes this computation unnecessarily complex. Decimals are either 16 or 32 bytes per the Arrow specification.
--
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] westonpace commented on a diff in pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #35183:
URL: https://github.com/apache/arrow/pull/35183#discussion_r1170189235
##########
csharp/test/Apache.Arrow.Tests/Decimal128ArrayTests.cs:
##########
@@ -41,7 +40,7 @@ public void AppendThenGetGivesNull()
var array = builder.Build();
Assert.Equal(3, array.Length);
- Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3);
+ Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3 + array.ByteWidth - 1);
Review Comment:
Decimal64 is a challenging type that has a pretty limited range. It doesn't cover very many gaps that double (double can perfectly represent 57 bits of decimal I believe) doesn't already take care of. But it isn't unheard of. In fact, some engines (e.g. I think clickhouse) have support for Decimal32.
There have been discussions in the past [1][2]. I think there would be general support for adding new narrow decimal types but no committer has been motivated to do that work.
[1] https://lists.apache.org/thread/wrgqzlr8pogl9674wgxjo6k12xl7n7cs
[2] https://lists.apache.org/thread/9ynjmjlxm44j2pt443mcr2hmdl7m43yz
--
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] Platob commented on a diff in pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "Platob (via GitHub)" <gi...@apache.org>.
Platob commented on code in PR #35183:
URL: https://github.com/apache/arrow/pull/35183#discussion_r1169532257
##########
csharp/test/Apache.Arrow.Tests/Decimal128ArrayTests.cs:
##########
@@ -41,7 +40,7 @@ public void AppendThenGetGivesNull()
var array = builder.Build();
Assert.Equal(3, array.Length);
- Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3);
+ Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3 + array.ByteWidth - 1);
Review Comment:
yeah indeed seeing cpp implementation its fixed
I saw in ParquetSharp implementation returning a Decimal128Type with 9 byteWidth for precision = 15 and scale = 5 which was different from 16
I wonder why its fixed since it could use way less memory for small numbers
--
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] Platob commented on a diff in pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "Platob (via GitHub)" <gi...@apache.org>.
Platob commented on code in PR #35183:
URL: https://github.com/apache/arrow/pull/35183#discussion_r1169532257
##########
csharp/test/Apache.Arrow.Tests/Decimal128ArrayTests.cs:
##########
@@ -41,7 +40,7 @@ public void AppendThenGetGivesNull()
var array = builder.Build();
Assert.Equal(3, array.Length);
- Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3);
+ Assert.Equal(array.Data.Buffers[1].Length, array.ByteWidth * 3 + array.ByteWidth - 1);
Review Comment:
yeah indeed seeing cpp implementation its fixed
I saw in ParquetSharp implementation returning a Decimal128Type with 9 byteWidth for precision = 15 and scale = 5 which was different from 16
--
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] Platob closed pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "Platob (via GitHub)" <gi...@apache.org>.
Platob closed pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
URL: https://github.com/apache/arrow/pull/35183
--
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] Platob commented on pull request #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "Platob (via GitHub)" <gi...@apache.org>.
Platob commented on PR #35183:
URL: https://github.com/apache/arrow/pull/35183#issuecomment-1512497592
Not conform with Arrow specs
--
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 #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35183:
URL: https://github.com/apache/arrow/pull/35183#issuecomment-1511159773
:warning: GitHub issue #35175 **has been automatically assigned in GitHub** to PR creator.
--
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 #35183: GH-35175: [C#] Decimal128 / 256 Types should alloc dynamic byteWidth based on precision and scale
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35183:
URL: https://github.com/apache/arrow/pull/35183#issuecomment-1511159708
* Closes: #35175
--
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