Skip to content

Conversation

@Krish-cloudsufi
Copy link
Owner

Key changes :-

  • Added Table Name in properties
  • Modified Schema retrieval using table name and importQuery


public List<SnowflakeFieldDescriptor> describeTable(String schemaName, String tableName) throws SQLException {
List<SnowflakeFieldDescriptor> fieldDescriptors = new ArrayList<>();

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.

Copy link
Owner Author

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

Copy link
Collaborator

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);

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,

Choose a reason for hiding this comment

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

maintain indentation

Repository owner deleted a comment from AbhishekKumar9984 Mar 28, 2025
Repository owner deleted a comment from AbhishekKumar9984 Mar 28, 2025
import io.cdap.plugin.snowflake.source.batch.SnowflakeBatchSourceConfig;
import io.cdap.plugin.snowflake.source.batch.SnowflakeInputFormatProvider;
import io.cdap.plugin.snowflake.source.batch.SnowflakeSourceAccessor;

Copy link
Collaborator

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) {
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this addressed?

Copy link
Owner Author

@Krish-cloudsufi Krish-cloudsufi Apr 15, 2025

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?

Copy link
Collaborator

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.")
Copy link
Collaborator

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

{
"name": "ImportQuery",
"condition": {
"expression": "importQueryType == 'importQuery'"
Copy link
Collaborator

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

"label": "Import Query",
"label": "Native Query",
"name": "importQuery",
"widget-attributes": {
Copy link
Collaborator

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

{
"widget-type": "textarea",
"label": "Import Query",
"label": "Native Query",
Copy link
Collaborator

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"
Copy link
Collaborator

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.

Copy link
Owner Author

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")
Copy link
Collaborator

@vikasrathee-cs vikasrathee-cs Apr 15, 2025

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

@sgarg-CS sgarg-CS Apr 16, 2025

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.

Copy link
Owner Author

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")
Copy link
Collaborator

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"
Copy link
Collaborator

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

Copy link
Owner Author

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",
Copy link
Collaborator

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.

Copy link
Owner Author

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

Copy link
Collaborator

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",
Copy link
Collaborator

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 ?

Copy link
Owner Author

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
Copy link
Collaborator

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

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.

5 participants