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

Implement postgres loader #23

Merged
merged 18 commits into from Oct 27, 2022
Merged

Implement postgres loader #23

merged 18 commits into from Oct 27, 2022

Conversation

rhazn
Copy link
Contributor

@rhazn rhazn commented Oct 27, 2022

closes #10

@rhazn rhazn changed the title WIP: Implement postgres loader Implement postgres loader Oct 27, 2022
@rhazn rhazn requested a review from felix-oq October 27, 2022 12:15
@@ -0,0 +1,39 @@
import { DataTypeVisitor } from './DataTypeVisitor';

export class PostgresValueRepresentationVisitor extends DataTypeVisitor<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a "weird" implementation right now. Essentially I return a function that takes an unknown value and returns a representation of that value (when looked at as being of the DataType) for postgresql.

I like this in the sense of the postgres specific part being in this visitor.

I dislike it for basically needing to handle all possibly "valid" values of a ValueType. This should probably be standardized by the valuetype first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to force valuetypes to provide a standardized representation of values first. I think this way is nicer.

@@ -62,7 +62,9 @@
"chevrotain": "^9.1.0",
"commander": "^8.0.0",
"fast-csv": "^4.3.6",
"fp-ts": "^2.13.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sadly also includes most of Georgs commits for error handling so it is easier to merge into main once we merge.

Copy link
Contributor

@felix-oq felix-oq left a comment

Choose a reason for hiding this comment

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

LGTM, I like the way you applied the visitor pattern to derive the postgres column types 🥇

We should further think about the distinction / combination of types for block in-/output in data-types.ts and the primitive types in AbstractDataType.ts. But that probably is beyond the scope of this PR.

@rhazn
Copy link
Contributor Author

rhazn commented Oct 27, 2022

I agree, I moved this to a new issue in #24 24

@rhazn rhazn merged commit 28a1f8a into main Oct 27, 2022
@rhazn rhazn deleted the issue-10 branch October 27, 2022 14:01
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.

Implement PostgresLoader
3 participants