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

feat: support all warehouses for tutorial #515

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: support all warehouses for tutorial #515

wants to merge 3 commits into from
+126 −182

Conversation

shubhammehra4
Copy link
Collaborator

@shubhammehra4 shubhammehra4 commented Nov 25, 2024 •
edited
Loading

Description of the change

  • Replace SnowPark connector with warehouse connector from pynative
  • Make all sql queries compatible with all warehouses

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Linear Ticket

Resolves PRML-1007

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@shubhammehra4 shubhammehra4 changed the title feat: support all warehouses feat: support all warehouses for tutorial Nov 25, 2024
shekhar-rudder
shekhar-rudder reviewed Nov 25, 2024
View reviewed changes
src/predictions/profiles_mlcorelib/py_native/profiles_tutorial/tutorial.py Outdated Show resolved Hide resolved
src/predictions/profiles_mlcorelib/py_native/profiles_tutorial/database_manager.py Outdated Show resolved Hide resolved
src/predictions/profiles_mlcorelib/py_native/profiles_tutorial/database_manager.py
table for table in tables if table in new_table_names.values()
]
tables = self.get_table_names()
new_tables = [new_table.lower() for new_table in new_table_names.values()]
Copy link
Member

Choose a reason for hiding this comment

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

Are we expecting the caller to pass new_table_names in lower case ? Otherwise, the if check will return false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we are lowecasing the table names

Copy link
Member

Choose a reason for hiding this comment

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

This can lead to bugs. Its better to handle it inside the function instead of expecting the callers to do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it

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

@dpatchigolla dpatchigolla Awaiting requested review from dpatchigolla

@shekhar-rudder shekhar-rudder Awaiting requested review from shekhar-rudder

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
2 participants
@shubhammehra4 @shekhar-rudder