Fix issues with longish concatenated context aliases#8494
Conversation
|
Test case is available upon request. |
asfernandes
left a comment
asfernandes
left a comment
There was a problem hiding this comment.
The adaptation of plans to the schemas PR was really trick.
Maybe you can do the master changes based on it or in the same branch?
| { | ||
| string truncatedAlias(alias); | ||
| truncatedAlias.resize(MAX_UCHAR - 3); | ||
| truncatedAlias += "..."; |
There was a problem hiding this comment.
How this change of a name overflow to ellipsis will resolve problems?
There was a problem hiding this comment.
It avoids an assertion (debug build) or weird alias truncation (release build) inside BlrWriter::appendString when called from genBlr(). Of course, we could just truncate the string at the 255 bytes boundary, but IMHO ellipsis look better as clearly shows the fact of truncation. Now BLR is generated (and later parsed) properly and ellipsis is shown inside the very long plan's alias, AFAIS without any additional side effects.
No problem. Once approved, I will commit to v5 and delay the master PR until schemas are merged. |
| void appendContextAlias(DsqlCompilerScratch* dsqlScratch, const string& alias) | ||
| { | ||
| const auto len = alias.length(); | ||
| if (len <= MAX_UCHAR) |
There was a problem hiding this comment.
Could it be more advantageous to use MAX_SQL_IDENTIFIER_LEN rather than MAX_UCHAR, considering the possibility of MetaName length expanding in the future?
There was a problem hiding this comment.
- Alias is a MetaName inside the query, but it's a string for the optimizer (because it's concatenated from the whole aliases stack for nested contexts like derived tables).
- String length inside BLR is limited by 1 byte, hence the check for MAX_UCHAR. It would be a PITA to extend it (without introducing new BLR version).
There was a problem hiding this comment.
Got it, thanks for the clarification.
|
::: QA note ::: |
4 participants
Currently nested contexts (e.g. derived tables) have context aliases concatenated through the whole stack, so the final string is not limited to the maximum metadata name length.
planEntry.aliasa string rather thanMetaNameAll these issues now assert: (1) and (2) for aliases longer than 252 bytes and (3) for aliases longer than 255 bytes.