Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Prototype ADQL support #70

wants to merge 1 commit into from

Conversation

xitology
Copy link
Member

No description provided.

@aplavin
Copy link
Contributor

aplavin commented Sep 20, 2024

This limit style does SELECT TOP n rendering, right?
I think it was introduced and popularized by Microsoft as part of their T-SQL dialect. Would probably be more understandable to call it :TSQL, it's definitely way more popular than ADQL :)

@xitology
Copy link
Member Author

This limit style does SELECT TOP n rendering, right? I think it was introduced and popularized by Microsoft as part of their T-SQL dialect. Would probably be more understandable to call it :TSQL, it's definitely way more popular than ADQL :)

Unfortunately, T-SQL syntax is not quite the same as ADQL's. This is because they handle OFFSET clause differently.

In ADQL (judging by the reference), OFFSET m is an independent clause that can be used alone or in combination with SELECT TOP n.

By contrast, older versions of T-SQL supported SELECT TOP n syntax, but had no way to skip initial rows, while newer versions support ANSI SQL compatible syntax ORDER BY ... OFFSET m ROWS. However T-SQL prohibits using both SELECT TOP n and OFFSET m ROWS in the same query and instead requires ANSI SQL syntax ORDER BY ... OFFSET m ROWS FETCH NEXT n ROWS.

@aplavin
Copy link
Contributor

aplavin commented Sep 23, 2024

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.

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a 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:

  1. Addition of Limit Styles:

    • The addition of new limit styles for ADQL, ANSI, MYSQL, and POSTGRESQL is a good step toward making the module flexible for handling various database dialects.
  2. 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.
  3. 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.
  4. Potential Enhancement:

    • The throw(DomainError(...)) at the end is important for handling unsupported styles. However, you might want to consider adding the new POSTGRESQL and MYSQL entries into the error message for full accuracy.
  5. 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.

Suggestions:

  • Refine Error Message:
    Update the error message inside DomainError 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants