Fix compiler error for lambda parameter with non-letter #5087
Conversation
| */ | ||
| private static String parameterName(int argumentPosition, String lambdaArgumentName) | ||
| { | ||
| return "lambda_" + argumentPosition + "_" + lambdaArgumentName.replaceAll("[^a-zA-Z_]", ""); |
There was a problem hiding this comment.
I realize "lambda_" + argumentPosition is enough from generated code perspective.
I retained argument name in case there is any need to debug the generated code.
There was a problem hiding this comment.
@Praveen2112 was there a similar escaping somewhere already? I think you did one, but I can't remember where was it.
There was a problem hiding this comment.
Yes !! It was in scalar function .. #2081
findepi
force-pushed
the
lamb
branch
2 times, most recently
from
b5484f3 to
973f08c
Compare
| */ | ||
| private static String parameterName(int argumentPosition, String lambdaArgumentName) | ||
| { | ||
| return "lambda_" + argumentPosition + "_" + lambdaArgumentName.replaceAll("[^a-zA-Z_]", ""); |
There was a problem hiding this comment.
@Praveen2112 was there a similar escaping somewhere already? I think you did one, but I can't remember where was it.
findepi
force-pushed
the
lamb
branch
2 times, most recently
from
4feb1eb to
d9508af
Compare
| /** | ||
| * Parameter name for the generated method. | ||
| */ | ||
| private static String parameterName(int argumentPosition, String lambdaArgumentName) |
There was a problem hiding this comment.
The current approach in the PR makes sense, considering that the construction of the class definition shouldn't make any assumptions about the symbols, and do this sanitization before creating parameter names.
Another place to do this sanitization could be SymbolAllocator#newSymbol. As a side-effect, it may improve plan readability (a minor benefit though). For example, if we truncate the string while "sanitizing" the symbols for the issue mentioned in #5088, the explain plan would display the truncated symbol. Thoughts?
There was a problem hiding this comment.
Another place to do this sanitization could be SymbolAllocator#newSymbol.
I thought about that. IMO the change here does not preclude change in SymbolAllocator.
And the change in SymbolAllocator does not make the change here obsolete:
compiler code should make as few assumptions about planner as possible.
It would feel wrong if planner had to consider Java class format constraints.
For #5088, as you pointed out, there are other benefits from keeping symbols short.
Plan readability, but also guard against any potential perf penalties
(at least in theory, a potential DOS attack vector).
Although the unqualified name in JVM apparently _can_ contain braces `(` or `)`, the intention was to remove them, retaining only letters, digits, underscore and dollar sign.
Fix a generated code error when lambda parameter contains a character that is not part of a valid Java identifier.
|
CI failed -- #4568 |
|
CI failed -- #4973 |
Fix a generated code error when lambda parameter contains a character
that is not part of a valid Java identifier.
Fixes #5085