Subject: [pgjdbc/pgjdbc] PR #3589: Metadata CHAR_OCTET_LENGTH is a positive number for non-character column types

From: "tirthgajera (@tirthgajera)" <[email-masked]>
Date: Sat, 05 Apr 2025 19:40:35 +0000

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing](https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md) document?
* [x] Have you checked to ensure there aren't other open [Pull Requests](../../pulls) for the same update/change?

<!-- You can erase any parts of this template not applicable to your Pull Request. -->

### New Feature Submissions:

1. [x] Does your submission pass tests?
2. [x] Does `./gradlew styleCheck` pass ?
3. [x] Have you added your new test classes to an existing test suite in alphabetical order?

### Changes to Existing Features:

* [x] Does this break existing behaviour? If so please explain.
* [ ] Have you added an explanation of what your changes do and why you'd like us to include them?
* [x] Have you written new tests for your core changes, as applicable?
* [x] Have you successfully run tests with your changes locally?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Sat, 05 Apr 2025 21:39:01 +0000

Please add a test for all of the cases

---

From: "tirthgajera (@tirthgajera)" <[email-masked]>
Date: Sun, 06 Apr 2025 12:30:25 +0000

> Please add a test for all of the cases

Done!! testcase added.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Sun, 06 Apr 2025 20:04:23 +0000

CHAR_OCTET_LENGTH returns an int, not a string

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 09 Apr 2025 18:00:57 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)

Could you add `NCHAR`, `NVARCHAR`, and `LONGNVARCHAR` as well?
It might be we should add `CLOB` here as well, however, I am not sure.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 09 Apr 2025 18:02:15 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/DatabaseMetaDataTest.java)

Great that you add assert messages, however, `expected null` does not add much since `assertNull` will add it anyways.

```suggestion
    assertNull(rs.getString("CHAR_OCTET_LENGTH"), "CHAR_OCTET_LENGTH for integer[]");
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 09 Apr 2025 18:02:46 +0000

Same thing, `expected 100` will appear in the message automatically.

```suggestion
    assertEquals("100", rs.getString("CHAR_OCTET_LENGTH"), "CHAR_OCTET_LENGTH for varchar(100)");
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 09 Apr 2025 18:04:19 +0000

The test method name looks outdated: it verifies CHAR_OCTET_LENGTH for several types, not just `char`.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 09 Apr 2025 18:05:12 +0000

Have you considered factoring out it to a helper method?

---

From: "tirthgajera (@tirthgajera)" <[email-masked]>
Date: Thu, 10 Apr 2025 09:38:00 +0000

need to add clob or?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 10 Apr 2025 09:55:48 +0000

why wouldn't we ?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Thu, 10 Apr 2025 10:11:33 +0000

The spec is not very clear. I would check the way MSSQL, and MySQL handles this.
However, CLOB "looks like text" and BLOB "looks like a binary", so it might be reasonable to add them both.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 10 Apr 2025 20:21:38 +0000

> The spec is not very clear. I would check the way MSSQL, and MySQL handles this. However, CLOB "looks like text" and BLOB "looks like a binary", so it might be reasonable to add them both.

I think it's fine to add them both as noted above

---

From: "tirthgajera (@tirthgajera)" <[email-masked]>
Date: Sat, 12 Apr 2025 13:13:39 +0000

Hi @davecramer , @vlsi,
Changes pushed!!

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:19:05 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/DatabaseMetaDataTest.java:1971)

Please move `create table` to `@BeforeAll`, and `drop table` to `@AfterAll`. It would reduce the number of drop/create during the test.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:21:21 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/DatabaseMetaDataTest.java:1991)

A tiny note: lambdas could help with lazy execution, so the test does not spend time on building the failure message in case of the passing tests.

```suggestion
      assertTrue(rs.next(), () -> "dbmd.getColumns returned no rows for column " + columnName);
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Sun, 13 Apr 2025 08:24:56 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/DatabaseMetaDataTest.java:1999)

The failure would maks the root cause. In most of the cases, I want to know the exact expected/actual value rather than just the fact "it should not be null".

Have you considered something like the following?

```java
Integer actual = rs.wasNull() ? null : rs.getInt("CHAR_OCTET_LENGTH");
assertEquals(expected, actual, ...);
```

---

From: "tirthgajera (@tirthgajera)" <[email-masked]>
Date: Fri, 18 Apr 2025 21:34:24 +0000

In test we already have BeforeEach and AfterEach. Is it okay if I add create and drop table there.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 18 Apr 2025 21:37:09 +0000

Why not put it in @BeforeAll and @AfterAll ?

---

From: "tirthgajera (@tirthgajera)" <[email-masked]>
Date: Fri, 18 Apr 2025 21:53:40 +0000

Yes, we can.
I'm introducing new beforeAll and afterAll for this test and adding it to it.