Support configurable delimiter for seed files, default to comma (#3990) #7186

ramonvermeulen · 2023-03-17T13:52:45Z

resolves #3990

Original description

Added support to configure a custom delimiter for seed files, defaults to comma.

Copilot description

This pull request introduces several changes related to seed files and configuration options. The most important changes include adding new test classes and methods to test different scenarios related to seed files with unique delimiters, adding a new configuration option for specifying a delimiter for seed files, and modifying various functions to support the new delimiter configuration option.

Summary of changes:

tests/adapter/dbt/tests/adapter/simple_seed/test_seed.py: Added a new test class and several test methods to test seed files with unique delimiters. [1] [2] [3]
core/dbt/contracts/graph/model_config.py: Added a new configuration option delimiter to the SeedConfig class.
tests/functional/artifacts/expected_manifest.py: Added a new configuration option delimiter to a function in expected_manifest.py.
tests/unit/test_contracts_graph_parsed.py: Added new configuration options delimiter to different functions. [1] [2] [3]

Other changes:

tests/adapter/dbt/tests/adapter/simple_seed/fixtures.py: Added a new model to the list of fixtures.
.changes/unreleased/Features-20230714-202445.yaml: Added support for configuring a delimiter for a seed file.
tests/functional/list/test_list.py: Added a new configuration option to a function.
core/dbt/context/providers.py: Added a new parameter to a function.
core/dbt/clients/agate_helper.py: Added a new parameter to a function.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR -> Add documentation to configure "delimiter" for seed files docs.getdbt.com#3015
I have run changie new to create a changelog entry

…labs#3990)

jtcohen6

Thanks @ramonvermeulen! I think we'd want a functional test for this as well, guaranteeing that a CSV with a nonstandard (non-comma) delimiter actually works.

From my POV, this doesn't feel super high-priority — dbt's seed functionality is intentionally limited / narrow in scope, and I hesitate before adding more, given the added maintenance burden — but this is a very straightforward & self-contained change. TSVs are common enough in the wild.

core/dbt/contracts/graph/model_config.py

jtcohen6 · 2023-03-22T12:35:58Z

core/dbt/clients/agate_helper.py

@@ -135,12 +135,12 @@ def as_matrix(table):
    return [r.values() for r in table.rows.values()]


-def from_csv(abspath, text_columns):
+def from_csv(abspath, text_columns, delimiter = ","):


Question: Should we add support for more than just delimiter here? Any/all **kwargs supported by agate.from_csv / Python's stdlib CSV reader?

Thoughts:

That can be out of scope for now / for this PR

It would risk locking us further into using agate (which I don't love), or at least the Python stdlib csv reader (which I don't mind as much)

ramonvermeulen · 2023-03-27T11:41:05Z

Thanks @ramonvermeulen! I think we'd want a functional test for this as well, guaranteeing that a CSV with a nonstandard (non-comma) delimiter actually works.

From my POV, this doesn't feel super high-priority — dbt's seed functionality is intentionally limited / narrow in scope, and I hesitate before adding more, given the added maintenance burden — but this is a very straightforward & self-contained change. TSVs are common enough in the wild.

Hi @jtcohen6, sorry for the late reply, I had a week of vacation.
I think adding a functional test is a good idea! I will dive into the functional test set-up and see if I can make a good functional test.

The reason I made this PR is because I encountered customers delivering there "csv" files with for example pipes or tabs as delimiter. Maybe because that is a standard within their organization. I thought it would be nice to have some more control about the configuration of the csv parser for the seed files, but I get your point about that it will increase the maintenance.

Question: Should we add support for more than just delimiter here? Any/all **kwargs supported by agate.from_csv / Python's stdlib CSV reader?

Thoughts:

That can be out of scope for now / for this PR

It would risk locking us further into using agate (which I don't love), or at least the Python stdlib csv reader (which I don't mind as much)

I find this a hard one, it is possible to add support for all the parameters of the agate.from_csv function, however this would lock dbt further in to using agate. The delimiter parameter, is an parameter that is also supported by the stdlib csv reader. I would say support for other parameters is out of scope for now.

JCZuurmond

LGTM, @ramonvermeulen!

core/dbt/clients/agate_helper.py

Co-authored-by: Cor <[email protected]>

Conflicts: test/unit/test_contracts_graph_parsed.py

eliwoods · 2023-05-23T05:29:10Z

@ramonvermeulen I am rather interested in this PR, is there anything I can do to help get it over the line?

ramonvermeulen · 2023-05-23T07:03:17Z

@ramonvermeulen I am rather interested in this PR, is there anything I can do to help get it over the line?

Hi @eliwoods, I think functional tests are still required to get this PR over the line @jtcohen6? I've been quite busy lately so feel free to pick this up / contribute on this as you wish!

ramonvermeulen · 2023-06-23T12:05:41Z

@jtcohen6 @eliwoods @JCZuurmond I added a couple of functional tests for this feature, and fixed the failing seed related integration tests. Only one integration test called test_snapshot_hard_delete is failing on my local system. However this test is failing for me on the main branch as well, so I don't think it is related to this pull request.

thomas-vl

LGTM 🚀

QMalcolm

@ramonvermeulen Thank you for putting all this work it! I've looked it over and pulled it down for some manual testing. Every looks great ✨ Gonna run it through our CI workflow, and then we should be good to go 🙂

QMalcolm · 2023-07-14T17:03:39Z

Eek, foiled by mypy. Perhaps I should have run the pre-commit hooks locally while reviewing 🙃

So there are two classes of mypy issues we're seeing. One is some simple formatting changes, which are simple fixes. The other is the errors like core/dbt/cli/main.py:403: error: <nothing> has no attribute "command" [attr-defined]. These errors are actually due to a recently released version of click, specifically click 8.1.4. We've fixed this in core, thus to fix this I'd recommend merging main into this branch to get it up to date.

merge main into configurableDelimiterSeeds

ramonvermeulen · 2023-07-14T18:16:13Z

Eek, foiled by mypy. Perhaps I should have run the pre-commit hooks locally while reviewing upside_down_face

So there are two classes of mypy issues we're seeing. One is some simple formatting changes, which are simple fixes. The other is the errors like core/dbt/cli/main.py:403: error: <nothing> has no attribute "command" [attr-defined]. These errors are actually due to a recently released version of click, specifically click 8.1.4. We've fixed this in core, thus to fix this I'd recommend merging main into this branch to get it up to date.

Thanks for taking a look at my PR! Just pushed a commit merging the upstream main back into my branch, and ran pre-commit, so hopefully that fixes the CI.

QMalcolm · 2023-07-14T18:28:17Z

Thanks for taking a look at my PR! Just pushed a commit merging the upstream main back into my branch, and ran pre-commit, so hopefully that fixes the CI.

Absolutely! Thank you for the quick turn around with the fixes. Also, sorry that it took me so long to get to my initial review. It's been a busier few weeks for me than I anticipated 😅 I'm gonna kick off the CI again

QMalcolm

Re-approving 🙂

codecov · 2023-08-01T15:48:30Z

Codecov Report

Merging #7186 (847eb2d) into main (7872f6a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #7186   +/-   ##
=======================================
  Coverage   86.22%   86.22%           
=======================================
  Files         174      174           
  Lines       25515    25517    +2     
=======================================
+ Hits        22000    22002    +2     
  Misses       3515     3515

Files Changed	Coverage Δ
core/dbt/clients/agate_helper.py	`96.52% <100.00%> (ø)`
core/dbt/context/providers.py	`88.66% <100.00%> (+0.01%)`	⬆️
core/dbt/contracts/graph/model_config.py	`93.81% <100.00%> (+0.01%)`	⬆️

saviorand · 2023-08-18T13:22:23Z

Installed the pre-release version and trying this in my dbt_project.yml, doesn't seem to work.. Not sure what's the issue:

seeds:
  <project_name>:
    <seed_directory_name>:
      +delimiter: ";"

ramonvermeulen · 2023-08-18T17:01:16Z

Installed the pre-release version and trying this in my dbt_project.yml, doesn't seem to work.. Not sure what's the issue:
seeds:
  <project_name>:
    <seed_directory_name>:
      +delimiter: ";"

Hmmm, not sure what is going wrong. Currently I am leaving for holidays for a week, but will definitely look into this once I am back.

ramonvermeulen · 2023-08-31T12:08:25Z

Installed the pre-release version and trying this in my dbt_project.yml, doesn't seem to work.. Not sure what's the issue:
seeds:
  <project_name>:
    <seed_directory_name>:
      +delimiter: ";"

@saviorand
I just tried to reproduce this bug using dbt-core and dbt-postgres version 1.7.0b1 but I am not able to.

I created a new dbt project using dbt init and created the following seed file:

a|b|c
1|2|3
4|5|6
7|8|9

On location seeds/test_file.csv with config:

seeds:
  <project_name>:
    +delimiter: "|"

Works without problems

On location seeds/sales/test_file.csv with config:

seeds:
  <project_name>:
    <seed_directory_name>
      +delimiter: "|"

Also works without problems

Could you provide more info in reproducing this issue, maybe I am missing something?

FishtownBuildBot · 2023-09-25T20:18:54Z

Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#4126

CarMoreno · 2024-05-08T15:44:25Z

Great feature!!. Just one point: What about TSV files? In that case, the delimiter would be \t and won't work. I know it can be considered an edge case, I had to change my TSV file to use | as the delimiter character and my seed worked.

ramonvermeulen · 2024-05-08T18:21:57Z

Great feature!!. Just one point: What about TSV files? In that case, the delimiter would be \t and won't work. I know it can be considered an edge case, I had to change my TSV file to use | as the delimiter character and my seed worked.

Good edge case actually that I didn't think about yet, I think as of now it works for any 1 character delimiter. I am currently on a longer break, but might look into this once I get back to see if I can add support for tab delimited files as well.

Small Update: when I take a look at the from_csv docs from agate I would expect that it would work with \t as well, not sure what the reason is that it does not.

ramonvermeulen added 2 commits March 17, 2023 14:42

Support configurable delimiter for seed files, default to comma (dbt-…

596856b

…labs#3990)

Update Features-20230317-144957.yaml

c6b5b12

ramonvermeulen requested review from a team as code owners March 17, 2023 13:52

ramonvermeulen requested review from QMalcolm and nssalian March 17, 2023 13:52

cla-bot bot added the cla:yes label Mar 17, 2023

This was referenced Mar 17, 2023

Introduce "delimiter" parameter for seeds? Allow non-csv file types #3990

Closed

Add documentation to configure "delimiter" for seed files dbt-labs/docs.getdbt.com#3015

Closed

jtcohen6 reviewed Mar 22, 2023

View reviewed changes

jtcohen6 added Team:Language ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Mar 22, 2023

Moved "delimiter" to seed config instead of node config

381c3b3

JCZuurmond approved these changes Mar 29, 2023

View reviewed changes

core/dbt/clients/agate_helper.py Outdated Show resolved Hide resolved

ramonvermeulen and others added 3 commits March 29, 2023 13:21

Update core/dbt/clients/agate_helper.py

f44244c

Co-authored-by: Cor <[email protected]>

Merge commit '050161c78f6d9acc36d0b76bd54ffcd778b7a467'

ca4ff79

Conflicts: test/unit/test_contracts_graph_parsed.py

Update test_contracts_graph_parsed.py

0322113

Merge branch 'dbt-labs:main' into configurableDelimiterSeeds

c2ba7d1

ramonvermeulen requested a review from a team as a code owner May 26, 2023 14:35

ramonvermeulen requested review from emmyoop and VersusFacit and removed request for nssalian and a team May 26, 2023 14:35

ramonvermeulen added 4 commits June 23, 2023 09:54

Merge commit '533988233ecc1b2391d2d6139e1d6be095e2d6cd'

b258f79

fixed integration tests

20be12b

Added functional tests for seed files with a unique delimiter

f810942

Added docstrings

10d76f4

ramonvermeulen added 2 commits June 23, 2023 14:18

Added a test for an empty string configured delimiter value

f37e74a

whitespace

d1c8261

thomas-vl approved these changes Jun 28, 2023

View reviewed changes

kvanzuijlen approved these changes Jun 28, 2023

View reviewed changes

ramonvermeulen requested review from jtcohen6 and JCZuurmond June 29, 2023 06:32

QMalcolm approved these changes Jul 14, 2023

View reviewed changes

QMalcolm self-requested a review July 14, 2023 17:04

ramonvermeulen added 2 commits July 14, 2023 20:11

Merge pull request #1 from dbt-labs/main

82f15c4

merge main into configurableDelimiterSeeds

ran black

2902368

ramonvermeulen added 2 commits July 14, 2023 20:25

updated changie entry

ff66966

Update Features-20230317-144957.yaml

3083a6d

jtcohen6 removed the Team:Language label Jul 19, 2023

QMalcolm approved these changes Jul 31, 2023

View reviewed changes

Merge branch 'dbt-labs:main' into configurableDelimiterSeeds

847eb2d

QMalcolm merged commit 6130a6e into dbt-labs:main Aug 1, 2023

graciegoheen added the user docs [docs.getdbt.com] Needs better documentation label Sep 25, 2023

tlento mentioned this pull request Nov 6, 2023

Update typing-extensions version to >=4.4 #9012

Merged

5 tasks

tlento mentioned this pull request Feb 26, 2024

Update dbt-semantic-interfaces dependency to compatible range #9671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support configurable delimiter for seed files, default to comma (#3990) #7186

Support configurable delimiter for seed files, default to comma (#3990) #7186

ramonvermeulen commented Mar 17, 2023 •

edited by runleonarun

Loading

jtcohen6 left a comment

jtcohen6 Mar 22, 2023

ramonvermeulen commented Mar 27, 2023 •

edited

Loading

JCZuurmond left a comment

eliwoods commented May 23, 2023 •

edited

Loading

ramonvermeulen commented May 23, 2023

ramonvermeulen commented Jun 23, 2023 •

edited

Loading

thomas-vl left a comment

QMalcolm left a comment •

edited

Loading

QMalcolm commented Jul 14, 2023 •

edited

Loading

ramonvermeulen commented Jul 14, 2023 •

edited

Loading

QMalcolm commented Jul 14, 2023

QMalcolm left a comment

codecov bot commented Aug 1, 2023 •

edited

Loading

saviorand commented Aug 18, 2023

ramonvermeulen commented Aug 18, 2023

ramonvermeulen commented Aug 31, 2023 •

edited

Loading

FishtownBuildBot commented Sep 25, 2023

CarMoreno commented May 8, 2024

ramonvermeulen commented May 8, 2024 •

edited

Loading

Support configurable delimiter for seed files, default to comma (#3990) #7186

Support configurable delimiter for seed files, default to comma (#3990) #7186

Conversation

ramonvermeulen commented Mar 17, 2023 • edited by runleonarun Loading

Original description

Copilot description

Checklist

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 Mar 22, 2023

Choose a reason for hiding this comment

ramonvermeulen commented Mar 27, 2023 • edited Loading

JCZuurmond left a comment

Choose a reason for hiding this comment

eliwoods commented May 23, 2023 • edited Loading

ramonvermeulen commented May 23, 2023

ramonvermeulen commented Jun 23, 2023 • edited Loading

thomas-vl left a comment

Choose a reason for hiding this comment

QMalcolm left a comment • edited Loading

Choose a reason for hiding this comment

QMalcolm commented Jul 14, 2023 • edited Loading

ramonvermeulen commented Jul 14, 2023 • edited Loading

QMalcolm commented Jul 14, 2023

QMalcolm left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 1, 2023 • edited Loading

Codecov Report

saviorand commented Aug 18, 2023

ramonvermeulen commented Aug 18, 2023

ramonvermeulen commented Aug 31, 2023 • edited Loading

FishtownBuildBot commented Sep 25, 2023

CarMoreno commented May 8, 2024

ramonvermeulen commented May 8, 2024 • edited Loading

ramonvermeulen commented Mar 17, 2023 •

edited by runleonarun

Loading

ramonvermeulen commented Mar 27, 2023 •

edited

Loading

eliwoods commented May 23, 2023 •

edited

Loading

ramonvermeulen commented Jun 23, 2023 •

edited

Loading

QMalcolm left a comment •

edited

Loading

QMalcolm commented Jul 14, 2023 •

edited

Loading

ramonvermeulen commented Jul 14, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading

ramonvermeulen commented Aug 31, 2023 •

edited

Loading

ramonvermeulen commented May 8, 2024 •

edited

Loading