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.

Sign up for GitHub

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

Jump to bottom

Make DQX compatible with Serverless #147

Merged
merged 24 commits into from
Feb 3, 2025
Merged

Make DQX compatible with Serverless #147

merged 24 commits into from
Feb 3, 2025

Conversation

mwojtyczka
Copy link
Contributor

@mwojtyczka mwojtyczka commented Jan 30, 2025 •
edited
Loading

Changes

  • Added running integration tests on serverless compute in addition
  • Removed cache to rely on delta cache
  • Fixed col functions to be fully compatible with ANSI SQL used in serverless compute
  • Added missing documentation

Linked issues

Resolves #146 #120

Tests

  • manually tested
  • added unit tests
  • added integration tests

@mwojtyczka mwojtyczka requested a review from a team as a code owner January 30, 2025 09:31
@mwojtyczka mwojtyczka requested review from grusin-db and removed request for a team January 30, 2025 09:31
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 30, 2025 •
edited
Loading

✅ 114/114 passed, 1 skipped, 23m34s total

Running from acceptance #324

@alexott
Copy link
Contributor

alexott commented Jan 30, 2025

But profiler tests will fail - should we remove .cache there?

@mwojtyczka
Copy link
Contributor Author

mwojtyczka commented Jan 30, 2025 •
edited
Loading

But profiler tests will fail - should we remove .cache there?

yes, this is going to be resolved in another PR. This one is to make the CI/CD work on serverless in addition.

@mwojtyczka
added tests on serverless
a65dff1 
added latest pytester dependency
@mwojtyczka mwojtyczka requested review from alexott and removed request for grusin-db January 31, 2025 23:45
@mwojtyczka mwojtyczka changed the title Added running integration tests on serverless compute Make DQX compatible with Serverless Jan 31, 2025
sundarshankar89
sundarshankar89 approved these changes Feb 3, 2025
View reviewed changes
Copy link
Collaborator

@sundarshankar89 sundarshankar89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mwojtyczka mwojtyczka merged commit 234b213 into main Feb 3, 2025
9 checks passed
@mwojtyczka mwojtyczka deleted the serverless branch February 3, 2025 12:20
@mwojtyczka mwojtyczka mentioned this pull request Feb 3, 2025
Closed
1 task
renardeinside
renardeinside reviewed Feb 3, 2025
View reviewed changes
Copy link
Contributor

@renardeinside renardeinside left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but minor nitpicks :)

docs/dqx/docs/dev/contributing.mdx
Comment on lines -102 to +110
# set either service principal credentials

# either use PAT token for authentication
export DATABRICKS_TOKEN=<pat-token>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't recommend PATs - they're considered bad practice since the introduction of OAuth

Copy link
Contributor Author

@mwojtyczka mwojtyczka Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, Thanks for the review, i will correct in the next PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renardeinside renardeinside renardeinside left review comments

@sundarshankar89 sundarshankar89 sundarshankar89 approved these changes

@alexott alexott Awaiting requested review from alexott

@canan-girgin canan-girgin Awaiting requested review from canan-girgin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Add execution of integration tests on serverless cluster
4 participants
@mwojtyczka @alexott @renardeinside @sundarshankar89