[GitHub] nifi issue #2424: NIFI-4393: Handle database specific identifier escape char...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2424 +1 LGTM merged. ---
[GitHub] nifi issue #2424: NIFI-4393: Handle database specific identifier escape char...
Github user patricker commented on the issue: https://github.com/apache/nifi/pull/2424 @ijokarumawak I think column names/table names should be kept unwrapped in all locations, and wrapped as needed by the Database Adapter. This will allow all columns to be stored uniformly no matter which adapter is used. Also in cases like `initial.maxvalue` property use, the user won't need to worry about wrapping the column name. ---
[GitHub] nifi issue #2424: NIFI-4393: Handle database specific identifier escape char...
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2424
@mattyb149 Thanks for your suggestion. I considered that path, too. But I
did it this way as it seems simpler to the objective to address the issue.
However, I think your comment is a good point. And I prefer having wrap method
in DatabaseAdapters instead of unwrap.
Let's discuss a bit more. I noticed that table and column names are spilled
into few places. I think we should define how we want those to be set.
- Processor properties:
- Columns to Return
- Maximum Value Columns
- Additional WHERE clause
- Custom SQL (to be added by #2162)
- Additional WHERE clause and Custom SQL might be challenging to wrap
problematic identifies by Processor automatically.
- Processor State: Stored with a key in `{tableName}!@!{columnName}`
format. Do we want to keep database specific characters here?
- Output FlowFile Attributes:
- max.{columnName} = {maxValue}
- tableName = {tableName]
- Do we want to keep database specific characters here?
Even if we change DatabaseAdapter method from unwrap to wrap, it's still
possible to use wrap character to unwrap. But we need to make a consensus on
how above listed variables are written.
---
[GitHub] nifi issue #2424: NIFI-4393: Handle database specific identifier escape char...
Github user mattyb149 commented on the issue: https://github.com/apache/nifi/pull/2424 Another approach, rather than "unwrapping" column names that are specified with dialect-specific characters in the properties, is to have them specified without the characters in the property, and the generated SQL would "wrap" them with the characters. This is the way I've seen it done before, by adding "getStartQuote" and "getEndQuote" methods to the DatabaseAdapter (possibly one set for table/DB names and one for column names in case they are specified differently in the dialect, I think at least one DB has different quotes for tables vs columns but I can't think of which one offhand). With the "wrap" approach, there are two downsides: 1) Users will have to change their properties to remove the special characters. However if this bug is saying that the special-character approach already doesn't work, then perhaps this is a non-issue. 2) We won't support column names that have commas in them. Again, we don't currently support this unless it happens to work with the special characters. The upside is that the property values would be more "natural", just a comma-separated list of column names (possibly with spaces or other values in them), and we don't have to rely on a regex to "unwrap" them, rather we would only "wrap" them internally when needed for the generated SQL. ---
[GitHub] nifi issue #2424: NIFI-4393: Handle database specific identifier escape char...
Github user pvillard31 commented on the issue: https://github.com/apache/nifi/pull/2424 Code LGTM @ijokarumawak. I'm a +1 on this PR but will wait a bit in case @mattyb149 or someone else wants to double check. ---
