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

Added specific pipes property constants #374

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

navinrathore
Copy link
Contributor

No description provided.

@navinrathore
Copy link
Contributor Author

Copy link
Member

@sonalgoyal sonalgoyal left a comment

Choose a reason for hiding this comment

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

I think we can just get rid of all the pipes in java and add the properties in python directly.

Pipe.__init__(self, name, "bigquery")
Pipe.addProperty(self, "viewsEnabled", "true")
Pipe.__init__(self, name, Format.BIGQUERY.type())
Pipe.addProperty(self, JBigQueryPipe.VIEWS_ENABLED, "true")
Copy link
Member

Choose a reason for hiding this comment

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

can we add this property by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Java app, there are no settings for BigQueryPipe,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Property is no more set here.

@@ -6,46 +6,48 @@

Format = jvm.zingg.client.pipe.Format
FilePipe = jvm.zingg.client.pipe.FilePipe
JSnowflakePipe = jvm.zingg.client.pipe.SnowflakePipe
JBigQueryPipe = jvm.zingg.client.pipe.BigQueryPipe

class CsvPipe(Pipe):
def __init__(self, name):
Pipe.__init__(self, name, Format.CSV.type())
Pipe.addProperty(self, FilePipe.HEADER,"true")
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong - we can not set header default true. we have to let the user set the values.

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've removed it now.

Copy link
Member

@sonalgoyal sonalgoyal left a comment

Choose a reason for hiding this comment

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

please add newlines between python methods.

@navinrathore
Copy link
Contributor Author

please add newlines between python methods.
Already in the code base
https://github.com/zinggAI/zingg/blob/main/python/zingg/pipes/pipes.py
through the below PR (for same comment here)
#376 (comment)

@navinrathore navinrathore marked this pull request as ready for review July 7, 2022 10:07
@navinrathore
Copy link
Contributor Author

Merge conflicts resolved.

@sonalgoyal sonalgoyal merged commit a694765 into zinggAI:main Jul 11, 2022
@navinrathore navinrathore deleted the PipeConstants branch July 28, 2022 15:58
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