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.

Sign up for GitHub

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

HCK-7055 - Fix sonarlint reports #219

Merged
merged 2 commits into from
Jul 12, 2024
Merged

HCK-7055 - Fix sonarlint reports #219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2d6625
Fix sonarlint reports
bigorn0 Jul 4, 2024
a2271fa
fix: removed proper duplication
chulanovskyi-bs Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 21 additions & 47 deletions reverse_engineering/hqlToCollectionsVisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,24 +74,19 @@ class Visitor extends HiveParserVisitor {
if (execStatement) {
return this.visit(execStatement);
}

return;
}

visitExecStatement(ctx) {
const ddlStatement = ctx.ddlStatement();
if (ddlStatement) {
return this.visit(ddlStatement);
}

return;
}

visitDdlStatement(ctx) {
if (ALLOWED_COMMANDS.includes(ctx.children[0].ruleIndex)) {
return super.visitDdlStatement(ctx)[0];
}
return;
}

visitCreateTableStatement(ctx) {
Expand Down Expand Up @@ -147,7 +142,7 @@ class Visitor extends HiveParserVisitor {
type: 'object',
properties: { ...convertKeysToProperties(compositePartitionKey, properties), ...properties },
}),
tableLikeName: (tableLikeName || {}).table,
tableLikeName: tableLikeName?.table,
entityLevelData: _.pickBy(
{
temporaryTable,
Expand Down Expand Up @@ -459,8 +454,6 @@ class Visitor extends HiveParserVisitor {
...this.visit(ctx.alterViewStatementSuffix()),
};
}

return;
}

visitAlterTableStatementSuffix(ctx) {
Expand All @@ -480,7 +473,7 @@ class Visitor extends HiveParserVisitor {
}

visitAlterStatementSuffixRename(ctx) {
const { database, table } = this.visit(ctx.tableName());
const { table } = this.visit(ctx.tableName());

return {
type: RENAME_COLLECTION_COMMAND,
Expand All @@ -497,8 +490,8 @@ class Visitor extends HiveParserVisitor {
type: UPDATE_ENTITY_LEVEL_DATA_COMMAND,
data: {
...this.visitWhenExists(ctx, 'tableSkewed', {}),
...(Boolean(ctx.KW_NOT() && ctx.KW_SKEWED()) ? { skewedby: [], skewedOn: '' } : {}),
...(Boolean(ctx.storedAsDirs()) ? { skewStoredAsDir: false } : {}),
...(ctx.KW_NOT() && ctx.KW_SKEWED() ? { skewedby: [], skewedOn: '' } : {}),
...(ctx.storedAsDirs() ? { skewStoredAsDir: false } : {}),
},
};
}
Expand Down Expand Up @@ -562,8 +555,8 @@ class Visitor extends HiveParserVisitor {
return {
type: UPDATE_ENTITY_LEVEL_DATA_COMMAND,
data: {
...(Boolean(ctx.KW_CLUSTERED()) ? { compositeClusteringKey: [] } : {}),
...(Boolean(ctx.KW_SORTED()) ? { sortedByKey: [] } : {}),
...(ctx.KW_CLUSTERED() ? { compositeClusteringKey: [] } : {}),
...(ctx.KW_SORTED() ? { sortedByKey: [] } : {}),
...this.visitWhenExists(ctx, 'tableBuckets', {}),
},
};
Expand All @@ -578,7 +571,7 @@ class Visitor extends HiveParserVisitor {
nameTo: this.visit(ctx.identifier()[1]),
data: {
...this.visit(ctx.colType()),
...(Boolean(ctx.KW_COMMENT()) ? { description: getTextFromStringLiteral(ctx) } : {}),
...(ctx.KW_COMMENT() ? { description: getTextFromStringLiteral(ctx) } : {}),
...columnConstraint,
},
};
Expand Down Expand Up @@ -649,10 +642,6 @@ class Visitor extends HiveParserVisitor {
}
}

visitAlterStatementSuffixRename(ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

deleted the wrong one 🙂 the last one declared takes precedence afaik, so this particular one was in use

return this.visit(ctx.tableName()).table;
}

visitAlterStatementSuffixAddConstraint(ctx) {
const foreignKey = ctx.alterForeignKeyWithName();
if (foreignKey) {
Expand Down Expand Up @@ -746,8 +735,6 @@ class Visitor extends HiveParserVisitor {
if (ctx.generatedAsIdentity()) {
return this.visit(ctx.generatedAsIdentity());
}

return;
}

visitGeneratedAsIdentity(ctx) {
Expand Down Expand Up @@ -973,7 +960,7 @@ class Visitor extends HiveParserVisitor {
const varchar = ctx.KW_VARCHAR();
const char = ctx.KW_CHAR();
if (timestampLocal) {
return { type: 'timestamp', mode: Boolean(ctx.KW_TIMESTAMP_NTZ()) ? 'timestamp_ntz' : '' };
return { type: 'timestamp', mode: ctx.KW_TIMESTAMP_NTZ() ? 'timestamp_ntz' : '' };
}
if (decimal) {
const decimalType = { type: 'numeric', mode: 'decimal' };
Expand Down Expand Up @@ -1054,12 +1041,12 @@ class Visitor extends HiveParserVisitor {

visitColumnConstraintType(ctx) {
return {
...(Boolean(ctx.KW_NOT() && ctx.KW_NULL()) ? { required: true } : {}),
...(Boolean((ctx.tableConstraintType() || {}).KW_UNIQUE) ? { unique: true } : {}),
...(Boolean((ctx.tableConstraintType() || {}).KW_PRIMARY) ? { primaryKey: true } : {}),
...(Boolean(ctx.KW_DEFAULT()) ? { default: this.visit(ctx.defaultVal()) } : {}),
...(Boolean(ctx.checkConstraint()) ? { check: this.visitWhenExists(ctx, 'checkConstraint', '') } : {}),
...(Boolean(ctx.columnGeneratedAs()) ? { generatedDefaultValue: this.visit(ctx.columnGeneratedAs()) } : {}),
...(ctx.KW_NOT() && ctx.KW_NULL() ? { required: true } : {}),
...(ctx.tableConstraintType?.KW_UNIQUE ? { unique: true } : {}),
...((ctx.tableConstraintType())?.KW_PRIMARY ? { primaryKey: true } : {}),
...(ctx.KW_DEFAULT() ? { default: this.visit(ctx.defaultVal()) } : {}),
...(ctx.checkConstraint() ? { check: this.visitWhenExists(ctx, 'checkConstraint', '') } : {}),
...(ctx.columnGeneratedAs() ? { generatedDefaultValue: this.visit(ctx.columnGeneratedAs()) } : {}),
};
}

Expand Down Expand Up @@ -1125,17 +1112,17 @@ class Visitor extends HiveParserVisitor {
}

visitTableFileFormat(ctx) {
if (Boolean(ctx.tableInputOutputFileFormat())) {
if (ctx.tableInputOutputFileFormat()) {
return {
storedAsTable: 'input/output format',
...this.visit(ctx.tableInputOutputFileFormat()),
};
} else if (Boolean(ctx.tableFileFormatStoredBy())) {
} else if (ctx.tableFileFormatStoredBy()) {
return {
storedAsTable: 'by',
...this.visit(ctx.tableFileFormatStoredBy()),
};
} else if (Boolean(ctx.tableFileFormatStoredAs())) {
} else if (ctx.tableFileFormatStoredAs()) {
return {
storedAsTable: this.visit(ctx.tableFileFormatStoredAs()),
};
Expand Down Expand Up @@ -1185,7 +1172,7 @@ class Visitor extends HiveParserVisitor {
visitCreateDatabaseStatement(ctx) {
const name = this.visit(ctx.identifier());
const description = this.visitWhenExists(ctx, 'databaseComment');
const locationPropertyName = Boolean(ctx?.dbLocation()?.KW_MANAGED()) ? 'managedLocation' : 'location';
const locationPropertyName = ctx?.dbLocation()?.KW_MANAGED() ? 'managedLocation' : 'location';
const locationValue = removeSingleDoubleQuotes(ctx?.dbLocation()?.StringLiteral()?.getText() || '');
const dbProperties = removeParentheses(ctx?.dbProperties()?.getText() || '');

Expand Down Expand Up @@ -1254,10 +1241,6 @@ class Visitor extends HiveParserVisitor {
};
}

visitColumnParenthesesList(ctx) {
chulanovskyi-bs marked this conversation as resolved.
Show resolved Hide resolved
return this.visit(ctx.columnNameList());
}

visitDropIndexStatement(ctx) {
const { database, table } = this.visit(ctx.tableName());
return {
Expand Down Expand Up @@ -1321,6 +1304,7 @@ class Visitor extends HiveParserVisitor {
};
}

// Check that this function is identical to the visitCreateBloomfilterIndexMainStatement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be assessed how you want to tackle that especially because semantically visiting the create and the drop statement share the exact same logic duplicated (not super obvious why because one could expect the must return different things...)

Copy link
Contributor

Choose a reason for hiding this comment

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

@bigorn0 We can move duplicated logic inside these two methods to a separate method in the visitor class, but having two entry points (visitDropBloomfilterIndexMainStatement and visitCreateBloomfilterIndexMainStatement) is required by ANTLR4 grammar - we should keep them.

Copy link
Contributor Author

@bigorn0 bigorn0 Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it is indeed required, shouldn't we expect different information extracted et returned by the visitor function for create vs drop?

Copy link
Contributor

@VitaliiBedletskyi VitaliiBedletskyi Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Both methods return objects that represent BloomfilterIndex even if it's a DROP statement. As I know originally it was a requirement to parse such statements (DROP... statements) and reverse them in the Hackolade model as deactivated objects. But the BloomfilterIndexes doesn't have the isActivated property - so as a result we have the identical logic and it's expected for now I supose.

visitDropBloomfilterIndexMainStatement(ctx) {
const { database, table } = this.visit(ctx.tableName());
return {
Expand Down Expand Up @@ -1359,7 +1343,7 @@ class Visitor extends HiveParserVisitor {
return {
type: CREATE_RESOURCE_PLAN,
name: this.visit(ctx.identifier()),
parallelism: (this.visitWhenExists(ctx, 'rpAssignList', {}) || {}).parallelism,
parallelism: this.visitWhenExists(ctx, 'rpAssignList', {})?.parallelism,
};
}

Expand Down Expand Up @@ -1445,17 +1429,6 @@ class Visitor extends HiveParserVisitor {
};
}

visitDropIndexStatement(ctx) {
const { database, table } = this.visit(ctx.tableName());

return {
type: REMOVE_COLLECTION_LEVEL_INDEX_COMMAND,
indexName: this.visit(ctx.identifier()),
bucketName: database,
collectionName: table,
};
}

visitDropViewStatement(ctx) {
const { database, view } = this.visit(ctx.viewName());

Expand All @@ -1466,6 +1439,7 @@ class Visitor extends HiveParserVisitor {
};
}

// Check that this function is identical to the one above
visitDropMaterializedViewStatement(ctx) {
const { database, view } = this.visit(ctx.viewName());

Expand Down
9 changes: 5 additions & 4 deletions reverse_engineering/thriftService/mapJsonSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,23 @@ const mapJsonSchema = _ => (jsonSchema, parentJsonSchema, callback, key) => {
const schema = mapper(jsonSchema[propertyName], propertyName);

if (_.isEmpty(schema)) {
const copySchema = Object.assign({}, jsonSchema);
const copySchema = {...jsonSchema};

delete copySchema[propertyName];

return copySchema;
}

return Object.assign({}, jsonSchema, {
return {
...jsonSchema,
[propertyName]: schema,
});
};
}, jsonSchema);
};
if (!_.isPlainObject(jsonSchema)) {
return jsonSchema;
}
const copyJsonSchema = Object.assign({}, jsonSchema);
const copyJsonSchema = {...jsonSchema};
const mapper = _.partial(mapJsonSchema(_), _, copyJsonSchema, callback);
const propertiesLike = ['properties', 'definitions', 'patternProperties'];
const itemsLike = ['items', 'not'];
Expand Down
Loading