-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prototype ADQL support #70
base: master
Are you sure you want to change the base?
Conversation
This limit style does |
Unfortunately, T-SQL syntax is not quite the same as ADQL's. This is because they handle In ADQL (judging by the reference), By contrast, older versions of T-SQL supported |
Oh, didn't know those differences! I think I only used TOP selection without any offsets, just to get a few rows and see if they look reasonable – before loading the full dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes made to the LIMIT_STYLE
module are well-structured and enhance the convert functionality by ensuring correct mappings for different SQL dialects. A few notes on the update:
-
Addition of Limit Styles:
- The addition of new limit styles for
ADQL
,ANSI
,MYSQL
, andPOSTGRESQL
is a good step toward making the module flexible for handling various database dialects.
- The addition of new limit styles for
-
Type Conversion Logic:
- The conversion logic uses a clean approach with symbol matching. This is an efficient way of converting symbols to their corresponding limit styles.
-
Consistency:
- You’ve ensured consistency in symbol names by including both lowercase and uppercase versions. This will help avoid potential mismatches or confusion when dealing with different naming conventions.
-
Potential Enhancement:
- The
throw(DomainError(...))
at the end is important for handling unsupported styles. However, you might want to consider adding the newPOSTGRESQL
andMYSQL
entries into the error message for full accuracy.
- The
-
Additional Testing:
- I would recommend adding unit tests to ensure that the
Base.convert
function handles all the limit styles correctly. This will help verify that the mappings and error handling work as expected.
- I would recommend adding unit tests to ensure that the
Suggestions:
-
Refine Error Message:
Update the error message insideDomainError
to include all supported SQL dialects, such as:throw(DomainError(QuoteNode(s), "expected :adql, :ansi, :mysql, :postgresql, :sqlserver"))
-
Testing:
Ensure comprehensive unit tests cover all supported limit styles and check edge cases for unsupported symbols.
No description provided.