mattcasters commented on code in PR #8782:
URL: https://github.com/apache/hop/pull/8782#discussion_r4210040508
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutputDialog.java:
##########
@@ -299,13 +314,40 @@ public void widgetSelected(SelectionEvent arg0) {
};
wTruncate.addSelectionListener(lsSelTMod);
+ // Do not create insert statements
+ wlDoNotAddInsertStmt = new Label(wFileName, SWT.RIGHT);
+ wlDoNotAddInsertStmt.setText(
+ BaseMessages.getString(PKG,
"SQLFileOutputDialog.DoNotAddInsertStatements.Tooltip")); // /!\
Review Comment:
**[bug]** The checkbox caption is loaded from
`SQLFileOutputDialog.DoNotAddInsertStatements.Tooltip` instead of `.Label`.
Both bundles define a short label ("Do not add 'insert into' statements"), but
the dialog puts the long tooltip in the right-aligned label, so it is clipped
next to the checkbox. The `// /!\` comment looks left in by mistake. That same
tooltip, and the user manual, also say this can emit TRUNCATE, but
`activateTruncate()` clears and disables truncate whenever "Add create table"
is checked, and this option is only enabled in that case, so the two cannot be
combined.
**Suggestion:** Use the `.Label` key for `setText` and keep the tooltip on
`setToolTipText`. Drop the truncate claim from the tooltip and the manual,
unless truncate is allowed to stay on together with this option.
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutput.java:
##########
@@ -68,8 +72,35 @@ public boolean processRow() throws HopException {
first = false;
data.outputRowMeta = getInputRowMeta().clone();
meta.getFields(data.outputRowMeta, getTransformName(), null, null, this,
metadataProvider);
- data.insertRowMeta = getInputRowMeta().clone();
+ if (meta.isSpecifyFields()
Review Comment:
**[suggestion]** If "Specify table fields" is checked and the grid is empty,
this condition fails and the transform silently uses every incoming field for
CREATE and INSERT. That contradicts the option and the manual ("only the
selected fields").
**Suggestion:** When `specifyFields` is true, honor the list even if it is
empty: fail with a clear error, or generate no columns. Use the full input row
only when the option is off.
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutputDialog.java:
##########
@@ -924,6 +1120,32 @@ private void getTableName() {
}
}
+ private void get() {
+ try {
+ IRowMeta r = pipelineMeta.getPrevTransformFields(variables,
transformName);
+ if (r != null && !r.isEmpty()) {
+ BaseTransformDialog.getFieldsFromPrevious(
+ r,
+ wFields,
+ 1,
+ new int[] {1, 2}, // Target column indexes (1 = Name, 2 = Rename)
Review Comment:
**[suggestion]** `getFieldsFromPrevious(..., new int[] {1, 2}, ...)` copies
the incoming name into both the Name column and the Rename column. Rename is
documented as optional, and a non-empty rename is always written into the
generated column name, so Get fields freezes every SQL column to the current
stream name. Editing Name afterwards changes which stream field is read without
changing the SQL column.
**Suggestion:** Pass `new int[] {1}` so only the Name column is filled and
Rename stays empty unless the user sets it.
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutputMeta.java:
##########
@@ -104,6 +108,12 @@ public class SQLFileOutputMeta extends
BaseTransformMeta<SQLFileOutput, SQLFileO
@HopMetadataProperty(key = "StartNewLine")
private boolean startNewLine;
+ @HopMetadataProperty(groupKey = "fields", key = "field")
+ private List<SQLFileOutputField> sqlFileOutputFields = new ArrayList<>();
+
+ @HopMetadataProperty(key = "specifyFields")
+ private boolean specifyFields;
Review Comment:
**[bug]** `specifyFields` and the field list are only applied in
`SQLFileOutput.processRow()`. `getSqlStatements()` still builds CREATE TABLE
from the full incoming row, and the dialog SQL button calls that after
`getInfo()`, so it can show and run DDL for columns the user excluded or
renamed. `check()` still requires every incoming field to exist in the target
table under its original name, so a valid subset or rename is reported as
"Fields in input stream, not found in output table". `analyseImpact()` likewise
records a write for every incoming field.
**Suggestion:** When `specifyFields` is set, build the same selected row
meta as `processRow` (lookup by name, apply rename) and use it in
`getSqlStatements()`, `check()`, and `analyseImpact()`. Table Output already
does this for its SQL button and `check()`.
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutputField.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.pipeline.transforms.sqlfileoutput;
+
+import java.util.Objects;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.hop.metadata.api.HopMetadataProperty;
+
+@Getter
+@Setter
+public class SQLFileOutputField {
+
+ public SQLFileOutputField() {}
+
+ public SQLFileOutputField(SQLFileOutputField f) {
+ this.name = f.name;
+ this.rename = f.rename;
+ }
+
+ /** Select: Name of the selected field */
+ @HopMetadataProperty(
+ key = "name",
+ injectionKey = "FIELD_NAME",
+ injectionKeyDescription = "SelectValues.Injection.FIELD_NAME")
Review Comment:
**[suggestion]** `injectionKeyDescription` points at
`SelectValues.Injection.FIELD_NAME` and `SelectValues.Injection.FIELD_RENAME`.
Those keys are not in this plugin's bundle. Metadata injection resolves the
description with `BaseMessages.getString` on this class, so the UI shows
`!SelectValues.Injection.FIELD_NAME!`.
**Suggestion:** Add `SQLFileOutput.Injection.FIELD_NAME` and
`SQLFileOutput.Injection.FIELD_RENAME` to the English and French message
bundles and reference those keys. The same applies to the rename property below.
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutput.java:
##########
@@ -124,15 +155,27 @@ && getLinesOutput() > 0
}
try {
- String sql =
- data.db.getSqlOutput(schemaName, tableName, data.insertRowMeta, r,
meta.getDateFormat())
- + ";";
+ String sql = "";
+ if (!meta.isDoNotAddInsertStatements()) {
Review Comment:
**[suggestion]** Inserts are skipped whenever `doNotAddInsertStatements` is
true, even if `createTable` is false. The dialog clears the checkbox unless
"Add create table" is checked, but metadata injection or a hand-edited pipeline
still takes this branch and writes an empty file (no CREATE, no INSERT) while
`linesOutput` keeps increasing. The manual says the option is only available
together with create table.
**Suggestion:** Skip INSERT generation only when
`isDoNotAddInsertStatements()` and `isCreateTable()` are both true.
##########
plugins/transforms/sqlfileoutput/src/main/java/org/apache/hop/pipeline/transforms/sqlfileoutput/SQLFileOutputDialog.java:
##########
@@ -706,6 +765,84 @@ public void focusGained(org.eclipse.swt.events.FocusEvent
e) {
}
});
+ // Specify table fields
+ wSpecifyFields = new Button(wContentComp, SWT.CHECK);
+ wSpecifyFields.setText(
+ BaseMessages.getString(PKG,
"SQLFileOutputMeta.Content.SpecifyTableFields"));
+ PropsUi.setLook(wSpecifyFields);
+ FormData fdSpecifyFields = new FormData();
+ fdSpecifyFields.left = new FormAttachment(0, margin);
+ fdSpecifyFields.top = new FormAttachment(wGFormatEncodage, margin);
+ wSpecifyFields.setLayoutData(fdSpecifyFields);
+ wSpecifyFields.addSelectionListener(
+ new SelectionAdapter() {
+ @Override
+ public void widgetSelected(SelectionEvent e) {
+ activateFieldsTable();
+ input.setChanged();
+ }
+ });
+
+ Group wGFields = new Group(wContentComp, SWT.SHADOW_NONE);
+ PropsUi.setLook(wGFields);
+ wGFields.setText(BaseMessages.getString(PKG,
"SQLFileOutputMeta.Group.SelectFields"));
+
+ FormLayout groupFieldsLayout = new FormLayout();
+ groupFieldsLayout.marginWidth = 10;
+ groupFieldsLayout.marginHeight = 10;
+ wGFields.setLayout(groupFieldsLayout);
+
+ FormData fdGFields = new FormData();
+ fdGFields.left = new FormAttachment(0, margin);
+ fdGFields.top = new FormAttachment(wSpecifyFields, margin);
+ fdGFields.right = new FormAttachment(100, -margin);
+ fdGFields.bottom = new FormAttachment(100, -margin);
+ wGFields.setLayoutData(fdGFields);
+
+ wGet = new Button(wGFields, SWT.PUSH);
+ wGet.setText(BaseMessages.getString(PKG, "System.Button.GetFields"));
+ PropsUi.setLook(wGet);
+ FormData fdGet = new FormData();
+ fdGet.left = new FormAttachment(0, 0);
+ fdGet.bottom = new FormAttachment(100, 0);
+ wGet.setLayoutData(fdGet);
+ wGet.addListener(SWT.Selection, e -> get());
+
+ // Select fields table
+ ColumnInfo[] colinf =
+ new ColumnInfo[] {
+ new ColumnInfo(
+ BaseMessages.getString(PKG,
"SQLFileOutputMeta.Content.Fieldname"),
+ ColumnInfo.COLUMN_TYPE_CCOMBO,
+ new String[] {BaseMessages.getString(PKG,
"SQLFileOutputMeta.Content.Loading")},
+ false),
+ new ColumnInfo(
+ BaseMessages.getString(PKG,
"SQLFileOutputMeta.Content.RenameTo"),
+ ColumnInfo.COLUMN_TYPE_TEXT,
+ ValueMetaFactory.getValueMetaNames())
Review Comment:
**[nit]** The Rename column is `COLUMN_TYPE_TEXT`, but it is constructed
with `ValueMetaFactory.getValueMetaNames()`. That varargs constructor stores
data-type names as combo values, and a text column never shows them. It looks
like a copy of a type column.
**Suggestion:** Use the two-argument constructor: `new ColumnInfo(label,
ColumnInfo.COLUMN_TYPE_TEXT)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
