[Enhancement] information_schema.COLUMNS support complex type #33431
Conversation
Signed-off-by: Astralidea <[email protected]>
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| // This is used for information_schema.COLUMNS COLUMN_TYPE | ||
| public String toMysqlColumnTypeString() { | ||
| return "unknown"; | ||
| } | ||
| } |
There was a problem hiding this comment.
This review is based on the code snippet provided:
-
Comments: The comments should be more informative, describing not just where the methods are used but also what they do. E.g.,
// This method returns the MySQL data type string for information_schema.COLUMNS DATA_TYPE -
Constants: If "unknown" is a fallback value that could potentially be used in other places inside your codebase as well, consider making it a constant, e.g.,
public static final String UNKNOWN = "unknown";. Using a constant improves readability and makes your codebase easier to maintain. -
Method purpose: Right now both
toMysqlDataTypeString()andtoMysqlColumnTypeString()methods are returning "unknown". Without additional context, this suggests that either these methods aren't fully implemented yet or their purpose isn't clear. Before these methods get used, ensure they serve their intended purpose and update the return statement with proper dynamic values instead of a hardcoded "unknown". -
Unit Tests: These added methods should have corresponding unit tests that validate correct behavior.
-
Follow Code Conventions: Depending on the codebase's established policies or the language style guide used (Java in this case), make sure you're consistent in following those conventions. For example, you'd typically find a single line between methods in most Java style guides.
Again this review assumes absent elements such as exceptions handling etc., are handled elsewhere in your codebase.
| value = _desc_result.columns[_column_index].columnDesc.columnTypeStr; | ||
| } else { | ||
| value = type_to_string(_desc_result.columns[_column_index].columnDesc); | ||
| } | ||
| fill_column_with_slot<TYPE_VARCHAR>(column.get(), (void*)&value); | ||
| } | ||
| break; |
There was a problem hiding this comment.
The code changes appear correct and improve the program by checking if dataType or columnTypeStr are set before attempting to use them. However, some improvements can be made to enhance readability, maintainability, and safety.
- Use meaningful variable names:
Consider renaming _desc_result to something more descriptive that reflects what this variable holds.
- Check for null pointers:
While it seems that the dereferenced values may not be null based on the given context, it would add robustness to your code to check for null references on (*chunk)->get_column_by_slot_id(8) and (*chunk)->get_column_by_slot_id(16) calls.
- Avoid C-style casting of std::string in C++:
The (void*)&value style cast from std::string to void* isn't a good practice in modern C++. What you want to pass should be the internal char array of the string, so use value.c_str() instead.
- Extend the comments:
Consider providing more explanation in their comments above each section of code. This makes it easier for others (and yourself in the future) to understand the purpose of this piece of code.
- Code duplication:
The block of code that extracts values out with the fall back to to_mysql_data_type_string/type_to_string method for default value looks identical except for specific fields/accessors. This could potentially be abstracted into a function to comply with the DRY (Don't Repeat Yourself) principle.
| default: | ||
| return toSql(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The code is generally well written. It's formatted consistently with a lot of alignment, indicating it likely adheres to your style guide. There are a few suggestions that could help improve the code:
-
Handle
enumtype more explicitly: In bothtoMysqlDataTypeString()andtoMysqlColumnTypeString()methods, you provided a 'default' case to handle all other types that are not explicitly called out. This loose handling might not always be best because if the internal implementation changes and extra types were added, they would all come into the 'default' branch thus possibly causing issues.default: return type.toString().toLowerCase();
You could add an explicit handling for each known type and a default throw for unknown types.
-
Use
toString()directly in default case of new methods: You're calling.toLowerCase()on the enumeration name. The ENUM.name() method returns the name of this enum constant, exactly as declared in its enum declaration. So, the.toLowerCase()seems redundant unless there's a specific use-case to cover. -
Add comments to explain method logic and branches: Your existing comments are descriptive but don’t really describe what the method does or why certain steps are necessary within the function. They seem to reference another implementation spanning different files, which might raise maintainability concerns in the long run.
-
Use of value-specific mappings: Depending on how complex your system is or can become, it may be more maintainable to use a Map to store these explicit conversions instead of a switch block.
-
Unit tests: It's ideal to also include testing for these methods to ensure correctness and prevent potential regressions. If tests exist elsewhere, they should be updated to cover these changes too.
Signed-off-by: Astralidea <[email protected]>
|
Kudos, SonarCloud Quality Gate passed! |
[FE Incremental Coverage Report]✅ pass : 17 / 19 (89.47%) file detail
|
[BE Incremental Coverage Report]❌ fail : 6 / 8 (75.00%) file detail
|
|
@Mergifyio backport branch-3.2 |
|
@Mergifyio backport branch-3.1 |
✅ Backports have been created
|
|
@Mergifyio backport branch-3.0 |
|
@Mergifyio backport branch-2.5 |
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
Signed-off-by: Astralidea <[email protected]> (cherry picked from commit a1a8427)
Signed-off-by: Astralidea <[email protected]> (cherry picked from commit a1a8427)
Signed-off-by: Astralidea <[email protected]> (cherry picked from commit a1a8427)
Signed-off-by: Astralidea <[email protected]> (cherry picked from commit a1a8427) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/service/FrontendServiceImpl.java
Signed-off-by: Astralidea <[email protected]> (cherry picked from commit a1a8427)
Signed-off-by: Astralidea <[email protected]> (cherry picked from commit a1a8427)
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: