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.

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

Update Excel plugin with output support #260

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

JCotton1123
Copy link
Contributor

Example profiles.yaml:

     plugins:
        - module: excel
          alias: accounts_file
          config:
            output:
              engine: 'xlsxwriter'
              engine_kwargs:
                # See https://xlsxwriter.readthedocs.io/workbook.html#Workbook
                options:
                  # See https://xlsxwriter.readthedocs.io/working_with_memory.html#memory-perf
                  strings_to_numbers: true
                  remove_timezone: true
                  use_zip64: true
                  nan_inf_to_errors: true
              file: "files/accounts.xlsx"
              index: false  # Don't output row number
              header_styling: false  # Disable default Pandas header styling

Example model:

{{ config(materialized='external', plugin='accounts_file', overrides={'sheet_name': 'New Accounts'}) }}

SELECT *
FROM {{ ref('accounts') }}
WHERE status = 'NEW'

# Instead if we instantiated the writer in the constructor
# with mode = 'w', this would result in an existing file getting
# overwritten. This can happen if dbt test is executed for example.
if not hasattr(self, '_excel_writer'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: does this bit need a lock? I'm assuming it's possible for multiple threads to hit it at the same time.

@jwills
Copy link
Collaborator

jwills commented Sep 29, 2023

(don't worry about the MotherDuck test failure, that's expected until they upgrade to 0.9.0)

@JCotton1123
Copy link
Contributor Author

@jwills the one thing this PR is missing are tests. If you can provide some guidance there I am happy to implement some. Also, thanks again for taking the time to chat this morning.

@jwills
Copy link
Collaborator

jwills commented Oct 3, 2023

@JCotton1123 hey sorry for the lag here, I'm at a conference this week. Re: testing, I think the best move would be to extract the Excel-related bits of https://github.com/duckdb/dbt-duckdb/blob/master/tests/functional/plugins/test_plugins.py into a new test_excel.py file in that same directory which would add support for exercising the output-side of the plugin as well as the read-side that is exercised there now.

Take a look at the flow there and see if you're up for it; dbt functional tests are a little squirrely to understand, so if it's not clicking just let me know and I can add them in when I'm back around next week.

@JCotton1123 JCotton1123 marked this pull request as ready for review October 16, 2023 22:20
@JCotton1123
Copy link
Contributor Author

@jwills just getting back to this...I attempted to follow your suggestion for implementing a test. LMK if this needs any additional changes?

@jwills
Copy link
Collaborator

jwills commented Oct 17, 2023

@JCotton1123 this looks great, thank you so much! The mypy linting error looks legit worth fixing and then I'm not totally sure what's up with the plugin test (maybe another dep needs to migrate from test_plugins.py to test_excel.py?) I'm happy to take a crack at it if you're busy otherwise, you just need to allow commits from upstream committers.

@JCotton1123
Copy link
Contributor Author

@jwills I believe those issues should be resolved 🤞

@jwills jwills merged commit 0d5ac9d into duckdb:master Oct 18, 2023
29 of 30 checks passed
@jwills
Copy link
Collaborator

jwills commented Oct 18, 2023

@JCotton1123! Yay-- thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants