LMnet commented on code in PR #23496:
URL: https://github.com/apache/kafka/pull/23496#discussion_r4058660405
##########
connect/api/src/test/java/org/apache/kafka/connect/data/ValuesTest.java:
##########
@@ -157,6 +157,13 @@ public void
shouldParseStringsBeginningWithFalseAsStrings() {
assertEquals("false]", schemaAndValue.value());
}
+ @Test
+ public void shouldParseStringsBeginningWithNumberAsStrings() {
+ SchemaAndValue schemaAndValue = Values.parseString("1::2");
+ assertEquals(Type.STRING, schemaAndValue.schema().type());
+ assertEquals("1::2", schemaAndValue.value());
Review Comment:
Can we maybe add a bit more options to check? For example:
* `"1|2"`
* `"1,2"`
* `"-1}"`
* `"+1]"`
* `"1.5:2.5"`
##########
connect/api/src/main/java/org/apache/kafka/connect/data/Values.java:
##########
@@ -868,11 +868,15 @@ private SchemaAndValue parseNextToken(boolean embedded,
String token) {
return temporal;
}
}
- if (firstCharIsDigit || firstChar == '+' || firstChar == '-') {
- try {
- return parseAsNumber(token);
- } catch (NumberFormatException e) {
- // can't parse as a number
+ if (embedded || !parser.hasNext()) {
Review Comment:
The fix itself looks good 👍
##########
connect/api/src/main/java/org/apache/kafka/connect/data/Values.java:
##########
@@ -868,11 +868,15 @@ private SchemaAndValue parseNextToken(boolean embedded,
String token) {
return temporal;
Review Comment:
It may be considered out of scope, but the temporal branch has the same
problem: no check that all tokens are consumed. I leave it up to you to add
this fix in the same PR or create another 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
