Skip to content

Conversation

@sultaniman
Copy link
Contributor

@sultaniman sultaniman commented Apr 29, 2024

This PR addresses #1216 and allows duckdb(:memory:) request

TODO

  • Tests

@sultaniman sultaniman added bug Something isn't working community Issue from dlt Slack community labels Apr 29, 2024
@sultaniman sultaniman self-assigned this Apr 29, 2024
@netlify
Copy link

netlify bot commented Apr 29, 2024

Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit 876a55f
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/6639d7cf57e9d50008baa797
😎 Deploy Preview https://deploy-preview-1297--dlt-hub-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sultaniman sultaniman marked this pull request as ready for review April 29, 2024 12:25
@sultaniman sultaniman requested review from rudolfix and sh-rp April 29, 2024 12:25
@sultaniman sultaniman force-pushed the fix/duckdb-in-memory-mode branch 2 times, most recently from 8f51b49 to 02fb63c Compare April 30, 2024 07:56
@sultaniman sultaniman requested a review from sh-rp April 30, 2024 08:34
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

OK it looks like we have problem with documentation, not the code. We clearly say how to pass in memory duckdb to be used:

The destination accepts a duckdb connection instance via credentials, so you can also open a database connection yourself and pass it to dlt to use. :memory: databases are supported.

import duckdb
db = duckdb.connect()
p = dlt.pipeline(
  pipeline_name='chess',
  destination=dlt.destinations.duckdb(db),
  dataset_name='chess_data',
  full_refresh=False,
)

but then say the following:
A few special connection strings are supported:

  • :pipeline: creates the database in the working directory of the pipeline with the name quack.duckdb.
  • :memory: creates an in-memory database. This may be useful for testing.

what we should IMO do

  1. drop :memory: form docs
  2. instead of accepting :memory: let's raise an exception that says what to do instead

if we create the duckdb instance in memory, it will be lost after the data loading, right? even if we somehow avoid closing it, passing this as external db is IMO way more intuitive

@sultaniman sultaniman force-pushed the fix/duckdb-in-memory-mode branch from 24c829d to e391de4 Compare April 30, 2024 10:27
@sultaniman sultaniman requested a review from rudolfix April 30, 2024 11:36
@sultaniman sultaniman requested a review from rudolfix April 30, 2024 14:37
Copy link
Collaborator

@sh-rp sh-rp left a comment

Choose a reason for hiding this comment

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

one small suggested change, but it's good!
and one request: please don't mark comments as resolved, the commenter should do this once they are satisfied that you have made the necesary change

@sultaniman sultaniman force-pushed the fix/duckdb-in-memory-mode branch from e2ecb2d to affbea3 Compare May 2, 2024 09:27
@sultaniman sultaniman requested a review from sh-rp May 2, 2024 09:41
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

code looks good but documentation was only partially updated and IMO not reviewed before push.

thing that helps me is to read my diff in the PR before asking for a review. because now we need another round and the ticket is delayed again...

@sultaniman sultaniman force-pushed the fix/duckdb-in-memory-mode branch from 2886073 to e1449e3 Compare May 6, 2024 14:18
@sultaniman sultaniman requested a review from VioletM May 6, 2024 14:51
@sultaniman sultaniman requested a review from VioletM May 6, 2024 15:29
@sultaniman sultaniman force-pushed the fix/duckdb-in-memory-mode branch from 6f956b8 to d7a7909 Compare May 7, 2024 07:25
@sultaniman sultaniman requested a review from rudolfix May 7, 2024 08:18
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

LGTM!

@rudolfix rudolfix merged commit 30f0416 into devel May 7, 2024
@rudolfix rudolfix deleted the fix/duckdb-in-memory-mode branch May 7, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working community Issue from dlt Slack community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants