DanielLeens commented on PR #12606:
URL: https://github.com/apache/seatunnel/pull/12606#issuecomment-6143529588

   Thanks @SEZ9 for the extra pass on head `b7f4a7e4`. I compared it with my 
10-12 review to avoid duplicates and to be clear on where the two overlap.
   
   - My Issue 3 (unconditional quoting vs. schema-qualified / already-quoted 
names) is the same finding as SEZ9's Issue 2; the `schema.type` assertion in 
the new unit test is the concrete case. I agree with raising it as the main 
item to resolve.
   - My Issue 2 (document the new option) and SEZ9's Issue 6 (Javadoc for the 
option key and `quoteUserDefinedType`) are the same point.
   - SEZ9's Issue 5 (option now attached to every fall-through column through 
`PostgresTypeConverter.convert()`, immutable `singletonMap`, `options` taking 
part in column equality) is a valid observation that I did not list. I agree it 
is Low severity; limiting it to names that really need quoting would keep other 
columns byte-for-byte unchanged.
   - SEZ9's Issue 1 (`buildUpdateColumnSQL` at `PostgresDialect.java:438` still 
uses the unquoted `sourceType`) is correct as a code observation, and I should 
have listed it. I checked the reachability: the PostgreSQL CDC resolver in this 
head only emits add-column events for this path (no modify/change event comes 
out of the PG CDC source), so the ALTER COLUMN TYPE gap is latent today rather 
than something this CDC job can hit. I would rate it non-blocking, but a short 
note in the PR description (or a follow-up) would help the next reader.
   
   I have not reproduced anything new beyond that, and my conclusion stays as 
in my last review: Ready to merge after fixes, with the schema-qualified 
quoting question (Issue 3 / SEZ9 Issue 2) being the one I would like to see 
answered before merge. Happy to look again when you push an update.
   
   Also, the only CI entry on this head is the `Build` check in 
`action_required`, so there is no verification from CI yet; please check that 
Actions are enabled in your fork so the workflow can run for the new head.
   


-- 
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]

Reply via email to