-
Notifications
You must be signed in to change notification settings - Fork 326
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
Save Database connection as data link, SQL Server data link support #11343
Save Database connection as data link, SQL Server data link support #11343
Conversation
… _actual_ table name which may differ from input (SQLServer added # for temp)
Snowflake tests: |
Feature.Sample.if_supported_else_throw self.connection.dialect "limit" <| | ||
new_ctx = self.context.set_limit max_rows | ||
self.updated_context new_ctx | ||
new_ctx = self.context.set_limit max_rows |
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.
Why is this check removed?
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.
I was getting failures like:
Reason: An unexpected dataflow error (limit not yet supported for DB backend. Use .read to download the data and use this feature.) has been matched (at X:\NBO\enso\test\Table_Tests\src\Database\Common\Audit_Spec.enso:71:13-49).
at <enso> Feature.if_supported_else_throw<arg-2>(X:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Database\0.0.0-dev\src\Feature.enso:60:13-73)
when invoking DB_Table.read
.
I'm surprised it was working at all in the first place, because our read
by default relies on limit
. It is actually an interesting thing to investigate.
But overall - as currently implemented - limit
is an intrinsic part of the read
operation, so it must be supported if we want proper read
. Thus I don't think it should be hidden behind Sample
flag. We could hide it if we change how read
works, but I'm not convinced we should.
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.
Okay, so I figured it out.
The tests that were currently being ran for SQLServer just never called into DB_Table.read
directly... Most of the tests would do table.at "col" . to_vector
. This calls read but with parameter ..All_Rows
meaning the limit
is skipped.
But if I just call read
with default parameters, limit
is needed to make it work. IMO this makes limit
fundamental to the basic read capability. We can redo read to work differently if we want to, but for now I think it is OK to do this. Also implementing limit is not very complicated for new backends, so I think it should be fine-ish.
Pull Request Description
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.