You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/04/13 07:59:43 UTC
[GitHub] [pulsar] shibd opened a new pull request, #15153: Pulsar SQL support for Decimal data type(AVRO Schema)
shibd opened a new pull request, #15153:
URL: https://github.com/apache/pulsar/pull/15153
### Motivation
Current Pulsar SQL does not support Decimal data type(AVRO Schema), Trino does specify Decimal as one of the [supported](https://trino.io/docs/current/language/types.html#decimal) data types. Avro also has specifications to [support ](https://avro.apache.org/docs/current/spec.html#Decimal)it.
### Modifications
- Pulsar SQL support for Decimal data type(AVRO Schema).
- Add unit test.
### Documentation
Need to update docs?
- [x] `no-need-doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#discussion_r853174841
##########
pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/avro/PulsarAvroColumnDecoder.java:
##########
@@ -205,6 +207,13 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ if (columnType instanceof DecimalType) {
+ ByteBuffer buffer = (ByteBuffer) value;
+ byte[] bytes = new byte[buffer.remaining()];
+ buffer.get(bytes);
+ return new BigInteger(bytes).longValue();
Review Comment:
Thank your reply, you are right. I looked into it, the presto support rules for decimal were as follows:
```
When the precision <= 0, throw Exception.
When the precision > 0 and <= 18, use LongDecimalType. and mapping Long
When the precision > 18 and <= 36, use LongDecimalType. and mapping Slice
When the precision > 36, throw Exception.
```
Current, Presto support `0 < precision < 36` decimal type.
I fix it, please take a look.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1098885603
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1101035109
> @shibd I noticed we have a test for BigDecimal https://github.com/apache/pulsar/blob/master/tests/integration/src/test/java/org/apache/pulsar/tests/integration/schema/Schemas.java#L99, could you please help check why the test works?
@codelipenghui Hi, This test is test pulsar schema. It has nothing to do with pulsar SQL.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1101382401
> @codelipenghui Hi, This test is test pulsar schema. It has nothing to do with pulsar SQL.
Oh, I see. Thanks for the clarification.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#discussion_r852159688
##########
pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/decoder/avro/PulsarAvroColumnDecoder.java:
##########
@@ -205,6 +207,13 @@ public long getLong() {
return floatToIntBits((Float) value);
}
+ if (columnType instanceof DecimalType) {
+ ByteBuffer buffer = (ByteBuffer) value;
+ byte[] bytes = new byte[buffer.remaining()];
+ buffer.get(bytes);
+ return new BigInteger(bytes).longValue();
Review Comment:
We might get a negative value If the value of Decimal is over the max value of long
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1099766279
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on pull request #15153: Pulsar SQL support for Decimal data type(AVRO Schema)
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1098174215
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#discussion_r854006339
##########
pulsar-sql/presto-pulsar/src/test/java/org/apache/pulsar/sql/presto/decoder/avro/TestAvroDecoder.java:
##########
@@ -127,6 +130,9 @@ public void testPrimitiveType() {
"enumField", VARCHAR, false, false, "enumField", null, null, PulsarColumnHandle.HandleKeyValueType.NONE);
checkValue(decodedRow, enumFieldColumnHandle, message.enumField.toString());
+ PulsarColumnHandle decimalFieldColumnHandle = new PulsarColumnHandle(getPulsarConnectorId().toString(),
+ "decimalField", DecimalType.createDecimalType(4, 2), false, false, "decimalField", null, null, PulsarColumnHandle.HandleKeyValueType.NONE);
+ checkValue(decodedRow, decimalFieldColumnHandle, message.decimalField);
Review Comment:
Ok, i will add 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on pull request #15153: Pulsar SQL support for Decimal data type(AVRO Schema)
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1098629255
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#issuecomment-1098827656
@mattisonchao @codelipenghui @gaoran10 @Technoboy- PTAL, 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15153:
URL: https://github.com/apache/pulsar/pull/15153
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a diff in pull request #15153: Pulsar SQL support for Decimal data type
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #15153:
URL: https://github.com/apache/pulsar/pull/15153#discussion_r853937390
##########
pulsar-sql/presto-pulsar/src/test/java/org/apache/pulsar/sql/presto/decoder/avro/TestAvroDecoder.java:
##########
@@ -127,6 +130,9 @@ public void testPrimitiveType() {
"enumField", VARCHAR, false, false, "enumField", null, null, PulsarColumnHandle.HandleKeyValueType.NONE);
checkValue(decodedRow, enumFieldColumnHandle, message.enumField.toString());
+ PulsarColumnHandle decimalFieldColumnHandle = new PulsarColumnHandle(getPulsarConnectorId().toString(),
+ "decimalField", DecimalType.createDecimalType(4, 2), false, false, "decimalField", null, null, PulsarColumnHandle.HandleKeyValueType.NONE);
+ checkValue(decodedRow, decimalFieldColumnHandle, message.decimalField);
Review Comment:
Since there are two implementation of DecimalType: `ShortDecimalType` and `LongDecimalType`. And they are mapping two different type: `Long` and `Slice`. But it looks like only the Long type was tested here. It's better to add a test case for the Slice.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org