qingwei91 commented on PR #20140: URL: https://github.com/apache/flink/pull/20140#issuecomment-1208594479
Hi @libenchao , thanks for the review! Thanks for pointing out the flaw, you're right. 🙇 On your recommended approach, I believe `JdbcFilterPushdownVisitor` needs to produce strings or some equivalent data, so that it can be used by `JdbcDynamicTableSource::getScanRuntimeProvider`, how should I make use of PrepareStatement here? Maybe I am missing something? I think you're pointing out a fundamental issue with this PR, SQL statement generation has to be dialect-specific, and me trying to provide a default implementation might be a lost cause here. If we cannot go down the prepared statement route, I can think of 2 ideas: 1. Implement dialect-specific method that converts `ValueLiteralExpression` to SQL string literal, and have `JdbcFilterPushdownVisitor` to make use of it. We don't have to support all dialect in 1 go, and we can simply fallback to in-memory filter when its not supported. 2. Let `JdbcFilterPushdownVisitor` be dialect-specific, then every implementation will need to deal with dialect specific thing including literal stringification. This is similar to current approach, the only difference is that currently this PR provides a default implementation, where this option will stop doing it. My implementation is tested and used in Production for SQL Server. Maybe I can rename it and make it specific to SQL Server jdbc dialect? (SQL Server dialect is still opened #20235), so this will have a dependency to it. Likewise, we can fallback to in-memory filtering when dialect without an implementation. I think option 1 is less code, but probably more fiddly and easier to break. Option 2 is likely gonna be more code, but the separation is cleaner and less likely to break. Let me know your thoughts. 😄 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
