[CALCITE-6509] MongoAdapter throws 'Invalid JSON Number' exception if first character of field name is number #3896
Conversation
dssysolyatin
force-pushed
the
CALCITE-6509
branch
from
cf80bcc to
1ccd12e
Compare
|
| @@ -114,10 +114,14 @@ static String quote(String s) { | |||
| } | |||
|
|
|||
| private static boolean needsQuote(String s) { | |||
| for (int i = 0, n = s.length(); i < n; i++) { | |||
| if (!s.isEmpty() | |||
There was a problem hiding this comment.
The field name can contain '$' ? The new code doesn't cover when '$' is in the middle of the field like 'te$st'.
There was a problem hiding this comment.
It is valid to use $ in the middle of an identifier. It is invalid only when it is at the beginning of the string.
db.zip.aggregate({ $project: { te$st: "$zip_code" } })
There was a problem hiding this comment.
Yes. Looks good. Do you think we should add this case in a unit test?
There was a problem hiding this comment.
I don't think so, because $ in the middle will work in any case, whether quoted or unquoted. Instead of writing all possible tests for quoting, it would be better to rewrite the Mongo adapter to use MongoDB builders. This way, we can avoid such mistakes.
There was a problem hiding this comment.
The newly modified code and test cases do not cover the original code logic. Testing coverage is not enough for this PR
There was a problem hiding this comment.
Based on your reply, I know it works for me. But only focus on the code, I don't know.
There was a problem hiding this comment.
Probably the best option would be to find a specification for fieldName and add it as a comment. I will try to find it tomorrow. Because usually, people don't write tests to check if letters 'e', 'a', or 'b' are allowed in the middle of a string if the specification states that they are allowed.
There was a problem hiding this comment.
@NobiGo, could you please recheck the PR? Instead of fixing individual quoting cases, I decided to quote every identifier because MongoDB Extended JSON conforms to the JSON RFC. Therefore, it is safe to use double quotes.
dssysolyatin
force-pushed
the
CALCITE-6509
branch
3 times, most recently
from
785a13e to
bfc5f37
Compare
… first character of field name is number
dssysolyatin
force-pushed
the
CALCITE-6509
branch
from
bfc5f37 to
8ab2ef6
Compare
|
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
… first character of field name is number