fix(drilling): drill by pagination works with MSSQL data source

[sfirke]

SUMMARY

fixes #24072 in which Microsoft SQL Server requires an order-by column in order for pagination to succeed. In doing so, now the drilled data is always sorted by the 1st column in ascending order. Previously it retained the dataset's sort order.

Better fixes outside of my ability might be:

• Implement this on a per-database level
• Sort by row number or the equivalent to functionally do nothing and preserve the underlying data order
• Improve user controls for how drilled data displays, allowing the user to control which columns are shown and what the sort order is

But I think the harm introduced (re-sorting the drilled data) is outweighed by fixing this feature for MSSQL.

TESTING INSTRUCTIONS

• CI
• Sam deployed and manually tested the docker image

BEFORE (4.0.0rc1

Drilling.in.4.0.0.take.2-20240313_160744-Meeting.Recording.mp4

AFTER

Post-Patch.MSSQL.Drilling-20240313_160519-Meeting.Recording.mp4

ADDITIONAL INFORMATION

• Has associated issue: The second page of drill-to-detail with MSSQL data source errors"requires an order_by when using an OFFSET or a non-simple LIMIT clause" #24072

Description by Korbit AI

What change is being made?

Update the drill-to-detail functionality to ensure drill by pagination works with MSSQL data sources, including adjustments to test expectations and query ordering.

Why are these changes being made?

These changes fix an issue where the drill feature did not work correctly with MSSQL due to ordering issues, by integrating proper ordering logic in the query and aligning test data with the new expected behavior. This approach ensures compatibility with additional data sources such as MSSQL, while maintaining existing functionality.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.69%. Comparing base (ad9024b) to head (2e580d6).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27442      +/-   ##
==========================================
+ Coverage   67.39%   69.69%   +2.30%     
==========================================
  Files        1909     1909              
  Lines       74744    74744              
  Branches     8327     8327              
==========================================
+ Hits        50371    52095    +1724     
+ Misses      22323    20599    -1724     
  Partials     2050     2050              

Flag	Coverage Δ
javascript	57.22% <ø> (ø)
mysql	78.03% <100.00%> (ø)
postgres	78.14% <100.00%> (ø)
presto	53.69% <50.00%> (?)
python	83.06% <100.00%> (+4.77%)	⬆️
sqlite	77.57% <100.00%> (ø)
unit	56.68% <50.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[john-bodley]

@sfirke once this is ready for review would you mind adding the relevant reviewers?

[michael-s-molina]

@sfirke the CI error you're getting

superset/common/query_actions.py:156: error: List item 0 has incompatible type "Tuple[Union[AdhocColumn, str], bool]"; expected "Tuple[Union[AdhocMetric, str], bool]" [list-item]

is because you're passing a Column here

query_obj.orderby = [(query_obj.columns[0], True)]

instead of a Metric which is what OrderBy expects.

Column = Union[AdhocColumn, str]
Metric = Union[AdhocMetric, str]
OrderBy = tuple[Metric, bool]

[sfirke]

I'm stuck, it's unclear to me why this test is failing:

 =================================== FAILURES ===================================
___________ TestPostChartDataApi.test_chart_data_applied_time_extras ___________

self = <tests.integration_tests.charts.data.api_tests.TestPostChartDataApi testMethod=test_chart_data_applied_time_extras>

    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
    def test_chart_data_applied_time_extras(self):
        """
        Chart data API: Test chart data query with applied time extras
        """
        self.query_context_payload["queries"][0]["applied_time_extras"] = {
            "__time_range": "100 years ago : now",
            "__time_origin": "now",
        }
        rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data")
>       self.assertEqual(rv.status_code, 200)
E       AssertionError: 400 != 200

And why is test-postgres-presto passing but test-postgres-hive not? Update: Is that test just broken on master branch right now? I see it failed on #27520 😭

[rusackas]

Looks like this is pretty stuck. Maybe it needs a rebase? I'll convert it to draft for now, but mark it as ready if/when you think it might be.

[rusackas]

/korbit-review

[korbit-ai]

I've completed my review and didn't find any issues.

Suppressed issues based on your team's Korbit activity

​

This issue	Is similar to	Because
line 182: The code assumes query_obj.columns will always have at least one element, which could lead to an IndexError if the columns list is empty.	Missing empty array check for queriesResponse	Similar issues were not addressed in the past

When you react to issues (for example, an upvote or downvote) or you fix them, Korbit will tune future reviews based on these signals.

Files scanned

​

File Path	Reviewed
superset/superset_typing.py	✅
superset/common/query_actions.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

​

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Issue Categories

​

Category	Enabled
Documentation	✅
Logging	✅
Error Handling	✅
Readability	✅
Design	✅
Performance	✅
Security	✅
Functionality	✅

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ support@korbit.ai

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

[sfirke]

"Draft" sounds right, I'd like to see this through but won't get to it anytime soon

…

On Wed, Mar 19, 2025, 9:26 PM korbit-ai[bot] ***@***.***> wrote: ***@***.***[bot]* commented on this pull request. I've completed my review and didn't find any issues. Suppressed issues based on your team's Korbit activity ​ This issue Is similar to Because line 182 <https://github.com/apache/superset/pull/27442/files/908a3aab440ee1252a12cde2bf6d1e05b1cf223c..a995f08def74a6c86495288a2001b3bb435fe8ec#diff-7b0f273ec0cd03f2e1c6d3822dbf3573dfcd5c08635ae246a93fe371899bf048R182-R182> : The code assumes query_obj.columns will always have at least one element, which could lead to an IndexError if the columns list is empty. Missing empty array check for queriesResponse <#32025 (comment)> Similar issues were not addressed in the past When you react to issues (for example, an upvote or downvote) or you fix them, Korbit will tune future reviews based on these signals. Files scanned ​ File Path Reviewed superset/superset_typing.py ✅ superset/common/query_actions.py ✅ Explore our documentation <https://docs.korbit.ai> to understand the languages and file types we support <https://docs.korbit.ai/configuration/supported-languages> and the files we ignore <https://docs.korbit.ai/configuration/ignoring-files>. Need a new review? Comment /korbit-review on this PR and I'll review your latest changes. Korbit Guide: Usage and Customization Interacting with Korbit - You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR. - You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR. - Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR. - On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment. - Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts. Customizing Korbit - Check out our docs <https://docs.korbit.ai/configuration/organization-settings> on how you can make Korbit work best for you and your team. - Customize Korbit for your organization through the Korbit Console <https://app.korbit.ai/aa91ff46-6083-4491-9416-b83dd1994b51/settings>. Current Korbit Configuration General Settings ​ Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories ​ Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support - Tell us what you think of Korbit <https://rfn13ctpmt0.typeform.com/to/DOoeLyoN> - Schedule a call with our team <https://meetings.salesloft.com/korbittechnologies/korbitai> - Email us @ ***@***.*** Note Korbit Pro is free for open source projects 🎉 Looking to add Korbit to your team? Get started with a free 2 week trial here <https://www.korbit.ai?utm_source=github_open_source_messaging> — Reply to this email directly, view it on GitHub <#27442 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZYDEEFZICSAZBRP3BPHGL2VIKKVAVCNFSM6AAAAABZMHSTAOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMBQHAYTKOBQGQ> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[korbit-ai]

@sfirke I am looking at your pull request. The description will be updated shortly. In the meantime, please do not edit the description until I have finished writing mine.

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Unsafe Column List Access ▹ view

Files scanned

​

File Path	Reviewed
superset/superset_typing.py	✅
superset/common/query_actions.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

​

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Issue Categories

​

Category	Enabled
Documentation	✅
Logging	✅
Error Handling	✅
Readability	✅
Design	✅
Performance	✅
Security	✅
Functionality	✅

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ support@korbit.ai

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

[korbit-ai]

Unsafe Column List Access

Tell me more

What is the issue?

The code assumes query_obj.columns is non-empty when setting the orderby clause, which could lead to an IndexError if columns list is empty.

Why this matters

If no columns are present in the datasource or query object, accessing index 0 will crash the application with an IndexError.

Suggested change ∙ Feature Preview

Add a guard clause to check for columns before setting the orderby:

if query_obj.columns:
    query_obj.orderby = [(query_obj.columns[0], True)]
else:
    query_obj.orderby = []

Provide feedback to improve future suggestions

💬 Looking for more details? Reply to this comment to chat with Korbit.

[rusackas]

@sfirke is this still WIP?

[sfirke]

@rusackas yes, alas. I will still get to this sometime, but not imminently.

[sfirke]

@rusackas I have superseded this pull request with a new version, #34583, that is passing CI. It needs a review but maybe we can get this into 6.0 and 5.0.1!