Address review on Get Fields errors, PostgreSQL NUMERIC, and query fallback

- Re-throw progress-dialog exceptions after the shell is disposed, and drop
  the extra monitor.done() so a failed Get Fields still shows the error dialog
- Clamp PostgreSQL NUMERIC precision to at least the scale (PG < 15 rejects
  s > p); document the 2.20 NUMERIC(length, scale) meaning
- Do not run getQueryFieldsFallback twice when that path already failed
- Prefer Hop Timestamp over original JDBC DATE when generating SQL Server DDL
- Map unsized NUMERIC/DECIMAL parameters back to Integer

Author: mattcasters

core/src/main/java/org/apache/hop/core/database/Database.java

@@ -2292,7 +2292,15 @@ public IRowMeta getQueryFields(String sql, boolean param, IRowMeta inform, Objec
         fields = getQueryFieldsFallback(sql, param, inform, data);
       }
     } catch (Exception e) {
-      fields = getQueryFieldsFallback(sql, param, inform, data);
+      // Only recover from the prepared-statement / data-service paths. The else branch already
+      // ran the fallback; calling it again would execute the user's SQL twice.
+      if (databaseMeta.supportsPreparedStatementMetadataRetrieval() || isDataServiceConnection()) {
+        fields = getQueryFieldsFallback(sql, param, inform, data);
+      } else if (e instanceof HopDatabaseException hopDatabaseException) {
+        throw hopDatabaseException;
+      } else {
+        throw new HopDatabaseException(e);
+      }
     }
 
     // Store in cache!!
@@ -3485,6 +3493,10 @@ public IRowMeta getParameterMetaData(PreparedStatement ps) {
         IValueMeta val = DatabaseTypeMapper.getValueMeta(this, databaseMeta, column, false, false);
         if (val == null) {
           val = new ValueMetaNone(name);
+        } else if (isUnsizedExactNumeric(sqlType, precision, scale) && val.isNumber()) {
+          // Drivers often report NUMERIC/DECIMAL with precision 0 for untyped parameters.
+          // The mapper then yields a double-backed Number; the old parameter path used Integer.
+          val = new ValueMetaInteger(name);
         }
         par.addValueMeta(val);
       }
@@ -3496,6 +3508,10 @@ public IRowMeta getParameterMetaData(PreparedStatement ps) {
     return par;
   }
 
+  private static boolean isUnsizedExactNumeric(int sqlType, int precision, int scale) {
+    return (sqlType == Types.NUMERIC || sqlType == Types.DECIMAL) && precision <= 0 && scale <= 0;
+  }
+
   public int countParameters(String sql) {
     int q = 0;
     boolean quoteOpened = false;

core/src/main/java/org/apache/hop/core/database/DatabaseMeta.java

@@ -1096,8 +1096,10 @@ public String getQuotedSchemaTableCombination(
   }
 
   private String quoteSchema(String schemaName) {
-    // A composite "catalog.schema" is split whenever the caller passed one. supportsCatalogs()
-    // is a browsing flag and must not collapse an explicit catalog prefix into one identifier.
+    // A composite "catalog.schema" is split whenever the caller passed one, including on dialects
+    // that return supportsCatalogs() == false (jTDS SQL Server, Access, Gupta, Iris). That flag is
+    // a browsing hint and must not collapse mydb.dbo into the unresolvable identifier [mydb.dbo].
+    // A schema whose name itself contains a literal dot is vanishingly rare next to catalog.schema.
     int separatorIndex = schemaName.indexOf('.');
     if (separatorIndex > 0 && separatorIndex < schemaName.length() - 1) {
       String catalogName = schemaName.substring(0, separatorIndex);

core/src/test/java/org/apache/hop/core/database/DatabaseTest.java

@@ -164,6 +164,23 @@ void getParameterMetaDataMapsNvarcharAndNumeric() throws Exception {
     assertEquals(4, rowMeta.getValueMeta(1).getPrecision());
   }
 
+  @Test
+  void getParameterMetaDataMapsUnsizedNumericToInteger() throws Exception {
+    when(meta.getIDatabase()).thenReturn(new NoneDatabaseMeta());
+    ParameterMetaData parameterMetaData = mock(ParameterMetaData.class);
+    when(ps.getParameterMetaData()).thenReturn(parameterMetaData);
+    when(parameterMetaData.getParameterCount()).thenReturn(1);
+    when(parameterMetaData.getParameterType(1)).thenReturn(Types.NUMERIC);
+    when(parameterMetaData.getPrecision(1)).thenReturn(0);
+    when(parameterMetaData.getScale(1)).thenReturn(0);
+
+    Database db = new Database(log, variables, meta);
+    IRowMeta rowMeta = db.getParameterMetaData(ps);
+
+    assertEquals(1, rowMeta.size());
+    assertTrue(rowMeta.getValueMeta(0).isInteger());
+  }
+
   /**
    * When using getLookup calls there is no need to make attempt to retrieve row set metadata for
    * every call. That may bring performance penalty depends on jdbc driver implementation. For some

docs/hop-dev-manual/modules/ROOT/pages/database/column-types.adoc

@@ -45,10 +45,14 @@ Hop provides the plain JDBC behaviour; a dialect declares the parts it does diff
 
 Two of these carry a meaning that is easy to get wrong.
 
-`length` on a numeric type is the number of digits *before* the decimal, not the total.
-A database reports the total, so the scale has to come off.
-`NUMERIC(10,2)` is length 8, precision 2, and is written back as `NUMERIC(8+2, 2)`.
-Getting this backwards makes a column grow by its scale on every round trip.
+`length` on a numeric type is the *total* number of significant digits, the same value the database reports as precision.
+`NUMERIC(10,2)` is length 10, precision 2, and is written back as `NUMERIC(10, 2)` (or `DECIMAL(10,2)`).
+`numericLength()` on a `DatabaseColumn` is the digits before the decimal (8 for that example) and is only for dialect rules that test whether a declaration is possible.
+
+PostgreSQL used to emit `NUMERIC(length+precision, precision)` when Hop length still meant integer digits.
+From 2.20 it emits `NUMERIC(length, precision)`.
+A hand-authored Number field of length 10, precision 3 therefore generates `NUMERIC(10, 3)` rather than `NUMERIC(13, 3)`.
+If scale is larger than length, precision is widened so PostgreSQL before 15 does not reject the statement.
 
 `precision` of exactly 1 on a `TYPE_DATE` is a marker meaning the value is a date rather than a timestamp.
 It is not a digit count.

docs/hop-user-manual/modules/ROOT/pages/database/databases/postgresql.adoc

@@ -30,3 +30,11 @@ under the License.
 |Documentation | https://jdbc.postgresql.org/documentation/head/index.html[Documentation Link]
 |JDBC Url  | jdbc:postgresql://host:port/database
 |===
+
+== Numeric DDL in 2.20
+
+Hop Number/BigNumber *length* is the total number of significant digits (the database precision), not the digits before the decimal.
+
+Table Output and similar DDL therefore emit `NUMERIC(length, scale)`.
+A field authored as length 10, precision 3 generates `NUMERIC(10, 3)`.
+In 2.19 and earlier the PostgreSQL dialect added length and scale and emitted `NUMERIC(13, 3)` for that same metadata.

plugins/databases/mssql/src/main/java/org/apache/hop/databases/mssql/MsSqlServerDatabaseMeta.java

@@ -77,17 +77,18 @@ public List<IDatabaseTypeRule> getTypeRules() {
    * known; new Hop Date fields stay DATETIME, new Timestamp fields become DATETIME2.
    */
   static String dateTimeColumnType(IValueMeta valueMeta) {
+    // Hop type first: a DATE column the user converted to Timestamp must keep the time.
+    if (valueMeta.getType() == IValueMeta.TYPE_TIMESTAMP
+        || "datetime2".equalsIgnoreCase(valueMeta.getOriginalColumnTypeName())) {
+      return "DATETIME2";
+    }
     int original = valueMeta.getOriginalColumnType();
     if (original == Types.DATE) {
       return "DATE";
     }
     if (original == Types.TIME) {
       return "TIME";
     }
-    if (valueMeta.getType() == IValueMeta.TYPE_TIMESTAMP
-        || "datetime2".equalsIgnoreCase(valueMeta.getOriginalColumnTypeName())) {
-      return "DATETIME2";
-    }
     return "DATETIME";
   }
 

plugins/databases/mssql/src/test/java/org/apache/hop/databases/mssql/MsSqlServerTypeRulesTest.java

@@ -69,6 +69,13 @@ void aPlainTimestampWritesDatetime2() {
     assertEquals("DATETIME2", write(new ValueMetaTimestamp("COL")));
   }
 
+  @Test
+  void aTimestampConvertedFromDateKeepsTheTime() {
+    IValueMeta valueMeta = new ValueMetaTimestamp("COL");
+    valueMeta.setOriginalColumnType(Types.DATE);
+    assertEquals("DATETIME2", write(valueMeta));
+  }
+
   @Test
   void nvarcharRoundTrips() throws Exception {
     assertEquals("NVARCHAR(20)", write(column(Types.NVARCHAR, "nvarchar", 20, 20)));

plugins/databases/postgresql/src/main/java/org/apache/hop/databases/postgresql/PostgreSqlDatabaseMeta.java

@@ -468,16 +468,19 @@ public String getFieldDefinition(
         } else {
           if (length > 0) {
             if (precision > 0 || length > 18) {
-              // Numeric(Precision, Scale): Hop length is the total number of significant digits.
-              if (length > MAX_NUMERIC_PRECISION) {
+              // Numeric(p, s): Hop length is the total number of significant digits. PostgreSQL
+              // before 15 rejects s > p, so widen p when a field was authored with scale larger
+              // than length.
+              int numericPrecision = Math.max(length, precision);
+              if (numericPrecision > MAX_NUMERIC_PRECISION) {
                 // PostgreSQL refuses a declared precision above 1000 outright: "NUMERIC precision
                 // 1073741824 must be between 1 and 1000". A length that large only ever arrives
                 // from the CLOB_LENGTH marker, which means unbounded, and an unconstrained NUMERIC
                 // is exactly that: it holds 131072 digits before the point, far past anything a
                 // Hop value carries.
                 retval += "NUMERIC";
               } else {
-                retval += "NUMERIC(" + length + ", " + precision + ")";
+                retval += "NUMERIC(" + numericPrecision + ", " + precision + ")";
               }
             } else {
               if (length > 9) {

plugins/databases/postgresql/src/test/java/org/apache/hop/databases/postgresql/PostgreSqlDatabaseMetaTest.java

@@ -809,7 +809,7 @@ void testSqlStatements() {
         nativeMeta.getAddColumnStatement(
             "FOO", new ValueMetaNumber("BAR", -10, 7), "", false, "", false));
     assertEquals(
-        "ALTER TABLE FOO ADD COLUMN BAR NUMERIC(5, 7)",
+        "ALTER TABLE FOO ADD COLUMN BAR NUMERIC(7, 7)",
         nativeMeta.getAddColumnStatement(
             "FOO", new ValueMetaNumber("BAR", 5, 7), "", false, "", false));
     // An ALTER TABLE spells a column the way a CREATE TABLE does: through the dialect's type

ui/src/main/java/org/apache/hop/ui/core/database/dialog/GetQueryFieldsProgressDialog.java

@@ -72,7 +72,6 @@ public IRowMeta open() {
                 e, "Problem encountered determining query fields: " + e.toString());
           } finally {
             db.disconnect();
-            monitor.done();
           }
         };