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/12/12 15:45:12 UTC
[GitHub] [beam] ccciudatu opened a new pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
ccciudatu opened a new pull request #13539:
URL: https://github.com/apache/beam/pull/13539
For thrift classes generated with `java:beans` (i.e. private fields), the current code will fail to infer the `FieldValueTypeInformation`, as it wrongly expects the fields to be public.
This PR fixes the issue and changes the thrift compiler params to showcase this scenario.
----------------------------------------------------------------
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 merged pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
TheNeuralBit merged pull request #13539:
URL: https://github.com/apache/beam/pull/13539
----------------------------------------------------------------
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] ccciudatu commented on a change in pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
ccciudatu commented on a change in pull request #13539:
URL: https://github.com/apache/beam/pull/13539#discussion_r541639383
##########
File path: sdks/java/io/thrift/src/test/java/org/apache/beam/sdk/io/thrift/ThriftIOTest.java
##########
@@ -72,16 +72,16 @@ public void setUp() throws Exception {
byte[] bytes = new byte[10];
ByteBuffer buffer = ByteBuffer.wrap(bytes);
- TEST_THRIFT_STRUCT.testByte = 100;
- TEST_THRIFT_STRUCT.testShort = 200;
- TEST_THRIFT_STRUCT.testInt = 2500;
- TEST_THRIFT_STRUCT.testLong = 79303L;
- TEST_THRIFT_STRUCT.testDouble = 25.007;
- TEST_THRIFT_STRUCT.testBool = true;
- TEST_THRIFT_STRUCT.stringIntMap = new HashMap<>();
- TEST_THRIFT_STRUCT.stringIntMap.put("first", (short) 1);
- TEST_THRIFT_STRUCT.stringIntMap.put("second", (short) 2);
- TEST_THRIFT_STRUCT.testBinary = buffer;
+ TEST_THRIFT_STRUCT.setTestByte((byte) 100);
+ TEST_THRIFT_STRUCT.setTestShort((short) 200);
+ TEST_THRIFT_STRUCT.setTestInt(2500);
+ TEST_THRIFT_STRUCT.setTestLong(79303L);
+ TEST_THRIFT_STRUCT.setTestDouble(25.007);
+ TEST_THRIFT_STRUCT.setTestBool(true);
+ TEST_THRIFT_STRUCT.setStringIntMap(new HashMap<>());
+ TEST_THRIFT_STRUCT.getStringIntMap().put("first", (short) 1);
+ TEST_THRIFT_STRUCT.getStringIntMap().put("second", (short) 2);
+ TEST_THRIFT_STRUCT.setTestBinary(buffer);
Review comment:
The older `ThriftIO` tests relied on fields being public.
##########
File path: sdks/java/io/thrift/src/test/java/org/apache/beam/sdk/io/thrift/ThriftIOTest.java
##########
@@ -240,15 +240,15 @@ public void testThriftCoder() {
// Generate random string
String randomString = RandomStringUtils.random(10, true, false);
short s = (short) RandomUtils.nextInt(0, Short.MAX_VALUE + 1);
- temp.stringIntMap = new HashMap<>();
- temp.stringIntMap.put(randomString, s);
- temp.testShort = s;
- temp.testBinary = buffer;
- temp.testBool = RandomUtils.nextBoolean();
- temp.testByte = (byte) RandomUtils.nextInt(0, Byte.MAX_VALUE + 1);
- temp.testDouble = RandomUtils.nextDouble();
- temp.testInt = RandomUtils.nextInt();
- temp.testLong = RandomUtils.nextLong();
+ temp.setStringIntMap(new HashMap<>());
+ temp.getStringIntMap().put(randomString, s);
+ temp.setTestShort(s);
+ temp.setTestBinary(buffer);
+ temp.setTestBool(RandomUtils.nextBoolean());
+ temp.setTestByte((byte) RandomUtils.nextInt(0, Byte.MAX_VALUE + 1));
+ temp.setTestDouble(RandomUtils.nextDouble());
+ temp.setTestInt(RandomUtils.nextInt());
+ temp.setTestLong(RandomUtils.nextLong());
Review comment:
The older `ThriftIO` tests relied on fields being public.
----------------------------------------------------------------
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] ccciudatu commented on pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
ccciudatu commented on pull request #13539:
URL: https://github.com/apache/beam/pull/13539#issuecomment-743777333
R: @chrlarsen
----------------------------------------------------------------
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] ccciudatu commented on pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
ccciudatu commented on pull request #13539:
URL: https://github.com/apache/beam/pull/13539#issuecomment-743780012
I can revert all test changes if they're too noisy, as the fixed schema now works with both beans and no-beans generated code (and with any possible combination of thrift compiler options, as I think I tried them all already).
----------------------------------------------------------------
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] ccciudatu commented on pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
ccciudatu commented on pull request #13539:
URL: https://github.com/apache/beam/pull/13539#issuecomment-743775901
R: @TheNeuralBit Just realised that https://github.com/apache/beam/pull/13428 does not work with the `beans` thrift compiler option, which renders all fields private. The fix is to use `getDeclaredField()` instead of `getField()`, thus making no assumptions about fields visibility.
----------------------------------------------------------------
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] ccciudatu commented on a change in pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
ccciudatu commented on a change in pull request #13539:
URL: https://github.com/apache/beam/pull/13539#discussion_r541639464
##########
File path: sdks/java/io/thrift/src/test/java/org/apache/beam/sdk/io/thrift/ThriftIOTest.java
##########
@@ -240,15 +240,15 @@ public void testThriftCoder() {
// Generate random string
String randomString = RandomStringUtils.random(10, true, false);
short s = (short) RandomUtils.nextInt(0, Short.MAX_VALUE + 1);
- temp.stringIntMap = new HashMap<>();
- temp.stringIntMap.put(randomString, s);
- temp.testShort = s;
- temp.testBinary = buffer;
- temp.testBool = RandomUtils.nextBoolean();
- temp.testByte = (byte) RandomUtils.nextInt(0, Byte.MAX_VALUE + 1);
- temp.testDouble = RandomUtils.nextDouble();
- temp.testInt = RandomUtils.nextInt();
- temp.testLong = RandomUtils.nextLong();
+ temp.setStringIntMap(new HashMap<>());
+ temp.getStringIntMap().put(randomString, s);
+ temp.setTestShort(s);
+ temp.setTestBinary(buffer);
+ temp.setTestBool(RandomUtils.nextBoolean());
+ temp.setTestByte((byte) RandomUtils.nextInt(0, Byte.MAX_VALUE + 1));
+ temp.setTestDouble(RandomUtils.nextDouble());
+ temp.setTestInt(RandomUtils.nextInt());
+ temp.setTestLong(RandomUtils.nextLong());
Review comment:
The older `ThriftIO` tests relied on fields being public.
##########
File path: sdks/java/io/thrift/src/test/java/org/apache/beam/sdk/io/thrift/ThriftIOTest.java
##########
@@ -72,16 +72,16 @@ public void setUp() throws Exception {
byte[] bytes = new byte[10];
ByteBuffer buffer = ByteBuffer.wrap(bytes);
- TEST_THRIFT_STRUCT.testByte = 100;
- TEST_THRIFT_STRUCT.testShort = 200;
- TEST_THRIFT_STRUCT.testInt = 2500;
- TEST_THRIFT_STRUCT.testLong = 79303L;
- TEST_THRIFT_STRUCT.testDouble = 25.007;
- TEST_THRIFT_STRUCT.testBool = true;
- TEST_THRIFT_STRUCT.stringIntMap = new HashMap<>();
- TEST_THRIFT_STRUCT.stringIntMap.put("first", (short) 1);
- TEST_THRIFT_STRUCT.stringIntMap.put("second", (short) 2);
- TEST_THRIFT_STRUCT.testBinary = buffer;
+ TEST_THRIFT_STRUCT.setTestByte((byte) 100);
+ TEST_THRIFT_STRUCT.setTestShort((short) 200);
+ TEST_THRIFT_STRUCT.setTestInt(2500);
+ TEST_THRIFT_STRUCT.setTestLong(79303L);
+ TEST_THRIFT_STRUCT.setTestDouble(25.007);
+ TEST_THRIFT_STRUCT.setTestBool(true);
+ TEST_THRIFT_STRUCT.setStringIntMap(new HashMap<>());
+ TEST_THRIFT_STRUCT.getStringIntMap().put("first", (short) 1);
+ TEST_THRIFT_STRUCT.getStringIntMap().put("second", (short) 2);
+ TEST_THRIFT_STRUCT.setTestBinary(buffer);
Review comment:
The older `ThriftIO` tests relied on fields being public.
----------------------------------------------------------------
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] ccciudatu commented on a change in pull request #13539: [BEAM-11338] Addendum: handle private fields in generated thrift code
Posted by GitBox <gi...@apache.org>.
ccciudatu commented on a change in pull request #13539:
URL: https://github.com/apache/beam/pull/13539#discussion_r541698496
##########
File path: sdks/java/io/thrift/src/test/java/org/apache/beam/sdk/io/thrift/ThriftIOTest.java
##########
@@ -240,15 +240,15 @@ public void testThriftCoder() {
// Generate random string
String randomString = RandomStringUtils.random(10, true, false);
short s = (short) RandomUtils.nextInt(0, Short.MAX_VALUE + 1);
- temp.stringIntMap = new HashMap<>();
- temp.stringIntMap.put(randomString, s);
- temp.testShort = s;
- temp.testBinary = buffer;
- temp.testBool = RandomUtils.nextBoolean();
- temp.testByte = (byte) RandomUtils.nextInt(0, Byte.MAX_VALUE + 1);
- temp.testDouble = RandomUtils.nextDouble();
- temp.testInt = RandomUtils.nextInt();
- temp.testLong = RandomUtils.nextLong();
Review comment:
The ThriftIO tests used to rely on fields being public.
----------------------------------------------------------------
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