You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/05/05 07:37:52 UTC
[GitHub] [beam] rahul8383 opened a new pull request #11609: [BEAM-9887] Expected Exception when building Row with logical types w…
rahul8383 opened a new pull request #11609:
URL: https://github.com/apache/beam/pull/11609
…ith Invalid input
`schema.logicaltypes.FixedBytes` logical type expects an argument - the length of the byte[].
When an invalid input value (with length < expectedLength) is provided while building the Row with FixedBytes logical type, `IllegalArgumentException` is expected. But, the Exception is not thrown. The below code illustrates the behaviour:
```
Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
byte[] byteArray = {1, 2, 3, 4, 5};
Row row = Row.withSchema(schema).withFieldValue("char", byteArray).build();
System.out.println(Arrays.toString(row.getLogicalTypeValue("char", byte[].class)));
```
The above code prints "[1, 2, 3, 4, 5]" with length 5 to the console, whereas the expected length of FixedBytes, is 10.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Expected Exception when building Row with logical types w…
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-623909918
I have a couple of questions regarding the behaviour of logical types:
- Will this line ever get hit? If not, can we modify `FixedBytes` as a `PassThroughLogicalType`?: https://github.com/apache/beam/blob/34c58c42f14d2534a2f72f9194ddf9cc69138eea/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java#L78
- Can we consider that the input value provided is of BaseType, which we can convert to InputType and store in memory?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624149550
Hm so there are several ways of manually building a Row instance that provide different levels of runtime type-checking. `Row#addValues` explicitly validates everything, and `Row#attachValues` explicitly does not, for performance reasons. In SQL we have an option to switch between the two:
https://github.com/apache/beam/blob/34c58c42f14d2534a2f72f9194ddf9cc69138eea/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamAggregationRel.java#L354-L357
So we can have runtime type-checking for debugging, but then turn it off for performance.
I'm not sure how `withFieldValue` is intended to work. I'm not sure if the missing `toInputType(toBaseType(value))` for that code path is intentional or an oversight. Can you clarify @reuvenlax?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-626139746
If there are no comments, can we close this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624160514
@reuvenlax @TheNeuralBit
Thanks for the review.
Could you please clarify the questions that I have asked above?
I wanted to understand more about logical types for my PR #11581
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia removed a comment on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
iemejia removed a comment on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-623911388
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624906438
> This seems to be a holdover. Previously Row stored logical type values as their base type, so we probably called `toBaseType(toInputType(x))`.
Even before the code which changed `Row` to store logical type values instead of base values in memory, while building the row, the input value is expected to be of correct length.
https://github.com/apache/beam/blob/9f0cb649d39ee6236ea27f111acb4b66591a80ec/sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java#L658-L660
I think this issue is present since the introduction of `FixedBytes`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624792506
> In that case, there is no need to handle this else case right? as we are making sure that the input has expected length while building the Row.
`checkArgument` ensures that the condition is true, so we are rejecting byte arrays that are too short, and accepting anything longer than the fixed size. I don't think there's any capability that pads zeroes for short arrays, this logic just allows _longer_ byte arrays, and copies out the appropriate portion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624810068
Ahh ok. I'm sorry for being so dense, I see what you're saying now. In `toBaseType` we validate that the array's length == the fixed length:
https://github.com/apache/beam/blob/5e1571760b61b8ce247d5375b71c8df4d69d6409/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java#L67-L70
This is where the exception checked in your new tests is thrown.
So we never actually get into `toInputType` where we could accept a shorter byte array and zero-pad. And there doesn't seem to be any other way to set a value by base type with a shorter byte array and pad it.
This seems to be a holdover. Previously Row stored logical type values as their base type, so we probably called `toBaseType(toInputType(x))`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11609:
URL: https://github.com/apache/beam/pull/11609#discussion_r420336807
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/logicaltypes/LogicalTypesTest.java
##########
@@ -97,4 +99,19 @@ public void testNanosDuration() {
assertEquals(duration, row.getLogicalTypeValue(0, NanosDuration.class));
assertEquals(durationAsRow, row.getBaseValue(0, Row.class));
}
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testFixedBytesIllegalArgument() {
+ Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
+ byte[] byteArrayWithLengthFive = {1, 2, 3, 4, 5};
+ Row row = Row.withSchema(schema).withFieldValue("char", byteArrayWithLengthFive).build();
+ }
Review comment:
Could you add a test like this but with `addValues`?
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/logicaltypes/LogicalTypesTest.java
##########
@@ -97,4 +99,19 @@ public void testNanosDuration() {
assertEquals(duration, row.getLogicalTypeValue(0, NanosDuration.class));
assertEquals(durationAsRow, row.getBaseValue(0, Row.class));
}
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testFixedBytesIllegalArgument() {
+ Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
+ byte[] byteArrayWithLengthFive = {1, 2, 3, 4, 5};
+ Row row = Row.withSchema(schema).withFieldValue("char", byteArrayWithLengthFive).build();
+ }
+
+ @Test
+ public void testFixedBytes() {
+ Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
+ byte[] byteArray = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+ Row row = Row.withSchema(schema).withFieldValue("char", byteArray).build();
+ assertTrue(Arrays.equals(byteArray, row.getLogicalTypeValue("char", byte[].class)));
+ }
Review comment:
Since these tests are really checking `Row`'s verification, I think they would be better in `RowTest`. Could you move them there?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on a change in pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on a change in pull request #11609:
URL: https://github.com/apache/beam/pull/11609#discussion_r420486636
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/logicaltypes/LogicalTypesTest.java
##########
@@ -97,4 +99,19 @@ public void testNanosDuration() {
assertEquals(duration, row.getLogicalTypeValue(0, NanosDuration.class));
assertEquals(durationAsRow, row.getBaseValue(0, Row.class));
}
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testFixedBytesIllegalArgument() {
+ Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
+ byte[] byteArrayWithLengthFive = {1, 2, 3, 4, 5};
+ Row row = Row.withSchema(schema).withFieldValue("char", byteArrayWithLengthFive).build();
+ }
Review comment:
Added `RowTest.testLogicalTypeWithInvalidInputValueByFieldIndex`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
iemejia commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-623911388
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624360661
> We always convert logical types to their base type when serializing with SchemaCoder, and convert back to the input type when deserializing. Other than that I think the only time it should get called is when constructing a Row instance (unless you use attachValues).
In that case, there is no need to handle this `else` case right? as we are making sure that the input has expected length while building the Row.
https://github.com/apache/beam/blob/5e1571760b61b8ce247d5375b71c8df4d69d6409/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java#L77
Even if `attachValues` is used while building the Row and the provided input value is invalid(invalid length), during serialization in `SchemaCoder`, the input value cannot be converted to base type as it doesn't have expected length and an `IllegalArgumentException` will be thrown.
> Would this just be so that we're guaranteed to call `toInputType` whenever setting a value on Row? This PR accomplishes the same thing right?
Can we support this feature: depending on the type of the input value provided while building the Row, we can call `toInputType(toBaseType(inputValue))` or `toInputType(inputValue)` i.e. support for providing base value while building the Row. If both the InputType and BaseType are one and the same, we can directly call `toInputType(inputValue)`. I am thinking that this might be helpful for logical types like `FixedBytes` or `FixedLengthString`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 edited a comment on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 edited a comment on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624153984
@TheNeuralBit
`schemas.logicaltypes.LogicalTypesTest.testFixedBytesIllegalArgument` Test will fail even if `addValue()` method is used instead of `withFieldValue()` method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624153984
@TheNeuralBit
schemas.logicaltypes.LogicalTypesTest.testFixedBytesIllegalArgument Test will fail even if addValue() method is used instead of withFieldValue() method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] reuvenlax commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624156907
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Expected Exception when building Row with logical types w…
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-623905118
R: @reuvenlax @TheNeuralBit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624794519
Whoops sorry I completely misread that. We accept short byte arrays, and zero-pad them. We do _not_ accept long byte arrays so that we don't have to truncate them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624243528
> Will this line ever get hit?
We always convert logical types to their base type when serializing with SchemaCoder, and convert back to the input type when deserializing. Other than that I think the only time it should get called is when constructing a Row instance (unless you use attachValues).
> Can we consider that the input value provided is of BaseType, which we can convert to InputType and store in memory?
Would this just be so that we're guaranteed to call `toInputType` whenever setting a value on Row? This PR accomplishes the same thing right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624812424
I'd be +1 for just dropping the padding logic. I don't think it should be the responsibility of the LogicalType to coerce values like this. What do you think @reuvenlax?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on a change in pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on a change in pull request #11609:
URL: https://github.com/apache/beam/pull/11609#discussion_r420487951
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/logicaltypes/LogicalTypesTest.java
##########
@@ -97,4 +99,19 @@ public void testNanosDuration() {
assertEquals(duration, row.getLogicalTypeValue(0, NanosDuration.class));
assertEquals(durationAsRow, row.getBaseValue(0, Row.class));
}
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testFixedBytesIllegalArgument() {
+ Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
+ byte[] byteArrayWithLengthFive = {1, 2, 3, 4, 5};
+ Row row = Row.withSchema(schema).withFieldValue("char", byteArrayWithLengthFive).build();
+ }
+
+ @Test
+ public void testFixedBytes() {
+ Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
+ byte[] byteArray = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+ Row row = Row.withSchema(schema).withFieldValue("char", byteArray).build();
+ assertTrue(Arrays.equals(byteArray, row.getLogicalTypeValue("char", byte[].class)));
+ }
Review comment:
Moved the tests to RowTest.java
case in point!
How can I write `FixedBytes` test which tests the behaviour of appending zeros? To test this behaviour, the input value should have length < expectedLength. But, if the input value's length is less than expected length, an `IllegalArgumentException` is thrown while building the Row.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] reuvenlax edited a comment on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
reuvenlax edited a comment on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-624156907
@TheNeuralBit withFieldValue should replace addValues for most users. addValues is difficult and error prone and withFieldValues allows building a row based on named fields instead of positional fields.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rahul8383 commented on pull request #11609: [BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input
Posted by GitBox <gi...@apache.org>.
rahul8383 commented on pull request #11609:
URL: https://github.com/apache/beam/pull/11609#issuecomment-626140088
> I'd be +1 for just dropping the padding logic. I don't think it should be the responsibility of the LogicalType to coerce values like this. What do you think @reuvenlax?
@TheNeuralBit @reuvenlax I have taken care of this for `FixedBytes` and other standard logical types in #11581
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org