[BEAM-2151]Fix inconsistent mapping for SQL FLOAT#2886
[BEAM-2151]Fix inconsistent mapping for SQL FLOAT#2886xumingming wants to merge 4 commits intoapache:DSL_SQLfrom
Conversation
|
Hi @xumingmin , can you take a look at this one? |
| case FLOAT: | ||
| record.addField(idx, doubleCoder.decode(inStream, context)); | ||
| Double raw = doubleCoder.decode(inStream, nested); | ||
| Float actual = (raw == null) ? null : raw.floatValue(); |
There was a problem hiding this comment.
null should not appear here, it's already ignored in the start of loop;
| BeamSqlRowCoder coder = BeamSqlRowCoder.of(); | ||
| CoderProperties.coderDecodeEncodeEqual(coder, row); | ||
| } | ||
| } |
There was a problem hiding this comment.
can you also have a test case to cover decoder?
There was a problem hiding this comment.
CoderProperties.coderDecodeEncodeEqual actually will test both encode and decode, I will rename the test method name encode to some name more appropriate.
| <artifactId>hamcrest-all</artifactId> | ||
| <version>1.3</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
can this be removed? don't see it's used.
There was a problem hiding this comment.
It is indirectly depended by CoderProperties.coderDecodeEncodeEqual
There was a problem hiding this comment.
Version should be inherited; no need to specify it here.
|
Thanks @xumingming for your contribution. LGTM |
| <artifactId>hamcrest-all</artifactId> | ||
| <version>1.3</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Version should be inherited; no need to specify it here.
|
Retest this please |
|
Merged. Please close at your leisure. |
4 participants
This pull request is mainly to fix the SQL FLOAT mapping, previously it is mapped to Float in
BeamSQLRow, but mapped toDoubleinBeamSQLRowCoder, now we consistently map SQL FLOAT to java Float.When try to unit test this, found another bug in BeamSQLRow: the VARCHAR field is not encode/decoded correctly(mainly because OUTTER Context vs NESTED Context), so fixed this bug at the same time.