-
Notifications
You must be signed in to change notification settings - Fork 0
Added Table Name #1
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
base: develop
Are you sure you want to change the base?
Conversation
|
|
||
| public List<SnowflakeFieldDescriptor> describeTable(String schemaName, String tableName) throws SQLException { | ||
| List<SnowflakeFieldDescriptor> fieldDescriptors = new ArrayList<>(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't left unwanted blank spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting was present in the original code as well, to maintain readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single lines can be added not double blank lines
| } | ||
| return Strings.isNullOrEmpty(importQuery) ? null : getSchema(snowflakeAccessor, importQuery); | ||
| } catch (SchemaParseException e) { | ||
| return getSchema(snowflakeAccessor, tableName, importQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to wrap this in a ternary operator as in the main file.So that null exception will be handled properly.
| @Nullable String schema, | ||
| @Nullable String tableName) { | ||
| super( | ||
| accountName, database, schemaName, username, password, keyPairEnabled, path, passphrase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintain indentation
| import io.cdap.plugin.snowflake.source.batch.SnowflakeBatchSourceConfig; | ||
| import io.cdap.plugin.snowflake.source.batch.SnowflakeInputFormatProvider; | ||
| import io.cdap.plugin.snowflake.source.batch.SnowflakeSourceAccessor; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this blank line
| } | ||
| } | ||
|
|
||
| private static Schema getSchema(SnowflakeAccessor snowflakeAccessor, String tableName, String importQuery) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @nullable in front of tableName and importQuery where ever it can be null in input params
| @Name(PROPERTY_TABLE_NAME) | ||
| @Description("Name of the table to insert records into.") | ||
| @Macro | ||
| @Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required on sink side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tableName property was present in the original repository on sink side, should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, remove @nullable annotation
|
|
||
| @Name(PROPERTY_TABLE_NAME) | ||
| @Nullable | ||
| @Description("The table name.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a little more description here and in md file
widgets/Snowflake-batchsource.json
Outdated
| { | ||
| "name": "ImportQuery", | ||
| "condition": { | ||
| "expression": "importQueryType == 'importQuery'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this condition != 'tableName', in that case it will work for null in case of upgrade
widgets/Snowflake-batchsource.json
Outdated
| "label": "Import Query", | ||
| "label": "Native Query", | ||
| "name": "importQuery", | ||
| "widget-attributes": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no default value, remove this
widgets/Snowflake-batchsource.json
Outdated
| { | ||
| "widget-type": "textarea", | ||
| "label": "Import Query", | ||
| "label": "Native Query", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the label import query only
| "options": [ | ||
| { | ||
| "id": "importQuery", | ||
| "label": "Native Query" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you can keep the label as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| @Description("The name of the table used to retrieve the schema.") | ||
| private final String tableName; | ||
|
|
||
| @Name("importQueryType") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it macro and importQueryType should be Constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this plugin property to .md files well. Add to SnowflakeSinkConfig as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
| private final String tableName; | ||
|
|
||
| @Name("importQueryType") | ||
| @Description("Choose between Table Name or Import Query") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whether to select Table Name or Import Query to extract the data.
| }, | ||
| { | ||
| "id": "tableName", | ||
| "label": "Named Table" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the label name should be consistent. In Source widget, it's Named Table while in sink it's Table Name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the options we are keeping it as "Named Table" and for the tableName property it is kept as "Table Name".
| "name": "role" | ||
| }, | ||
| { | ||
| "name": "importQueryType", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the importQueryType radio button for Snowflake-batchsink.json as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not required on sink side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it's not required on the sink plugin, then you can ignore the above comment.
| "widget-type": "radio-group", | ||
| "widget-attributes": { | ||
| "layout": "inline", | ||
| "default": "importQuery", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While getting the schema, we're checking for the tableName first and then the importQuery, so shouldn't tableName be the default value for the importQueryType ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was asked to keep the importQuery as default
|
|
||
| @Name("importQueryType") | ||
| @Description("Whether to select Table Name or Import Query to extract the data.") | ||
| @Macro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it Nullable for upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Key changes :-