GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Sqlalchemy dev POC by overcoil · Pull Request #30 · databricks/databricks-sql-python

Conversation

overcoil commented Aug 10, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

What type of PR is this?

Feature

Description

This is a barebone dialect together with an expanded integration test that builds on the initial scaffolding. The main add are primary/foreign key support (to ignore them if they're specified) and explicit support for the numeric, string and date/time types.

How is this tested?

Unit tests
E2E Tests
[X ] Manually
N/A

I expanded integration.py to cover the scenario of creating and dropping a table with the types.

Related Tickets & Documents

PECO-174

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

susodapop and others added 30 commits

July 7, 2022 17:14


          Scaffold basic file structure for a new dialect

dd3cf40

https://github.com/sqlalchemy/sqlalchemy/blob/main/README.dialects.rst

Signed-off-by: Jesse Whitehouse <[email protected]>


          barebone (& non-working) DatabricksDialect implementation

e13fb3b

-a bare-bone dialect styled after PyHive and Impyla
-includes minimal/stubs for DatabricksIdentifierPreparer, DatabricksExecutionContext, DatabricksTypeCompiler, & DatabricksCompiler


          reminder about TABLE_SCHEM

0ee8d26


          initial checkin with working pytest test suites.

2742a92

-bare-bone implementation of dialect
-basic test suite
-sample app (not working)


          remove secret in comment

c6c1322

(secret revoked)


          minor corrections

a594e7c


          add cleanup and notes for self

595178b


          add sample programs & prelim README

d7e72a8


          add prelim support for string and derived types

59bf8b7


          tidy up for the week; pulled out partial interval support for the while

44a9e32


          add a block of test suites from SA

6463fef


          Trial add of Github Action for the SQLAlchemy dialect

d3bd5d2


          yml error


          add self-reference to trigger a run when the action is update

d75710a


          correct usage of env var

55e4448


          move to repo secrets instead

dc70f90


          correct drop pseudo-targets

d6f98dd


          add trigger on the top-level Makefile (convenience)

8e480ad


          add dbsqlcli for cleanup

d68ddb5


          add init invocation of dbsqlcli

c6a9386


          override the return code

01f5a16


          wrong conjunction!

d121881


          correct table name

55aa104


          restrict to the dev branch for the while


          various minor items

629a510

-clean up Makefile for pytest
-explore passing in --requirements to help with suite run
-exploration of DDLCompiler


          Merge branch 'sqlalchemy-dev' of https://github.com/overcoil/fork-dat…

01a4d00

…abricks-sql-python into sqlalchemy-dev


          Cleaned up and reformatted

dd79718

Signed-off-by: George Chow <[email protected]>


          removed unneeded cruft from earlier experiments

2a8b36d

Signed-off-by: George Chow <[email protected]>


          missed the other Makefile

1fcdfc2

Signed-off-by: George Chow <[email protected]>


          revert unneeded changes; remove dead template

488b94d

Signed-off-by: George Chow <[email protected]>

overcoil requested review from arikfr, moderakh, susodapop and yunbodeng-db as code owners

August 10, 2022 22:36

moderakh reviewed

Aug 12, 2022

View reviewed changes

Comment thread

.github/workflows/sqlalchemy-dialect.yml Outdated

		id: setup-python
		uses: actions/setup-python@v2
		with:
		python-version: 3.7

moderakh Aug 12, 2022

Copy link

Copy Markdown

Collaborator

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.

does this work on other versions of python3 as well? any limitation on the supported python version?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

It should. Note that I cribbed from code-quality-checks.yml for this action to test out some ideas. Now that I think about it, this action won't be operative since I've since removed the Pytest test. I will remove this action completely.

(The current plan is to not turn on Github Action in the OSS repo; contributors are encouraged to do so and to use these though.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

Rectified

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.github/workflows/sqlalchemy-dialect.yml Outdated

		DATABRICKS_SCHEMA: ${{ secrets.REPOSEC_DATABRICKS_SCHEMA }}

		run: \|
		echo y \| dbsqlcli --hostname $DATABRICKS_SERVER_HOSTNAME --http-path $DATABRICKS_HTTP_PATH --access-token $DATABRICKS_TOKEN -e "USE $DATABRICKS_SCHEMA; DROP TABLE IF EXISTS t; DROP TABLE IF EXISTS tabletest; DROP TABLE IF EXISTS integer_table;"

moderakh Aug 12, 2022

Copy link

Copy Markdown

Collaborator

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.

what happens if there are concurrent PRs? are they going to use the same database name and table name and hence one PR CI may fail because of other PR CI writing to the same table?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

Refer to my answers to your other comment on this action.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/databricks/sqlalchemy/requirements.py

		@@ -1,18 +0,0 @@
		# Following official SQLAlchemy guide:

moderakh Aug 12, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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.

where did this class come from? did we already have some sql-alchemy related code in the main branch?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

This was created initially from the rough scaffolding that Jesse and I put together earlier on (commit 0378460). I ended up tossing it after I figured out that I won't be using SA's test suite.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/databricks/sqlalchemy/dialect.py

		if type_.precision is None:
		return "DECIMAL"
		elif type_.scale is None:
		return "DECIMAL(%(precision)s)" % {"precision": type_.precision}

moderakh Aug 12, 2022

Copy link

Copy Markdown

Collaborator

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.

do we have test covering the methods of this class?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

Good catch. I will revisit integration.py to include both the default and specified scales.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

Rectified.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/databricks/sqlalchemy/dialect.py

		supports_unicode_statements = True
		supports_unicode_binds = True
		returns_unicode_strings = True
		description_encoding = None

moderakh Aug 12, 2022

Copy link

Copy Markdown

Collaborator

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.

what's description_encoding?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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.

According to what I found in SA's doc (https://github.com/sqlalchemy/sqlalchemy/blob/main/doc/build/changelog/changelog_05.rst), it "is used for encoding the column name when processing the metadata. This usually defaults to utf-8." I went with "None" as for all the other dialects I studied.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/databricks/sqlalchemy/dialect.py

		# provide a way to debug
		debugbreakpoint = os.getenv("DATABRICKS_DIALECT_DEBUG") or False

		class DatabricksDialect(default.DefaultDialect):

moderakh Aug 12, 2022

Copy link

Copy Markdown

Collaborator

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.

seems src/databricks/sqlalchemy/dialect.py already existed before. Where did this come from?

did we have some sort of support for sqlalchemy dialect previously?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

overcoil Aug 12, 2022

Copy link

Copy Markdown

Author

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 pre-existing src/databricks/sqlalchemy/dialect.py was a rough scaffolding that Jesse and I put together earlier on (see commit 0378460). I built on this scaffolding.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          remove unneeded action; expand decimal test case to cover default and…

7ba1f2a

… specified scales

Signed-off-by: George Chow <[email protected]>

susodapop commented Oct 14, 2022

Copy link

Copy Markdown

Contributor

Closing in favour of #57 although I will incorporate many of these tests into that PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

susodapop closed this

Oct 14, 2022

Reviewers

moderakh

moderakh left review comments

susodapop

Awaiting requested review from susodapop

arikfr

Awaiting requested review from arikfr

yunbodeng-db

Awaiting requested review from yunbodeng-db

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

overcoil commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moderakh Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susodapop commented Oct 14, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

overcoil commented Aug 10, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

moderakh Aug 12, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.