-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Param source timezone (review) #374
Changes from 1 commit
e8cef17
035a9d1
592c2e9
6e2b139
2c83aac
7b67d47
716d968
504ee45
7de7462
ad07054
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,14 @@ | |
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import org.apache.commons.lang3.StringUtils; | ||
import java.sql.ResultSet; | ||
import java.sql.SQLException; | ||
import java.util.LinkedHashMap; | ||
import java.util.Map; | ||
import java.util.Properties; | ||
import java.util.regex.Pattern; | ||
import java.util.regex.Matcher; | ||
|
||
public class BaseDbWriter { | ||
|
||
|
@@ -75,7 +78,7 @@ protected void createConnection(String url, String clientName, String userName, | |
* Function that uses the DatabaseMetaData JDBC functionality | ||
* to get the column name and column data type as key/value pair. | ||
*/ | ||
public Map<String, String> getColumnsDataTypesForTable(String tableName) { | ||
public Map<String, String> getColumnsDataTypesForTable(String tableName, String sourceTimeZone) { | ||
|
||
LinkedHashMap<String, String> result = new LinkedHashMap<>(); | ||
try { | ||
|
@@ -89,6 +92,9 @@ public Map<String, String> getColumnsDataTypesForTable(String tableName) { | |
while (columns.next()) { | ||
String columnName = columns.getString("COLUMN_NAME"); | ||
String typeName = columns.getString("TYPE_NAME"); | ||
if (typeName.contains("DateTime")) { | ||
typeName = addTimeZoneToColumnDefinition(typeName, sourceTimeZone); | ||
} | ||
|
||
// Object dataType = columns.getString("DATA_TYPE"); | ||
// String columnSize = columns.getString("COLUMN_SIZE"); | ||
|
@@ -141,6 +147,29 @@ public ResultSet executeQueryWithResultSet(String sql) throws SQLException { | |
|
||
} | ||
|
||
public String addTimeZoneToColumnDefinition(String typeName, String timeZone) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure it is a good idea to use a regexp here as the target DateTime64 may already have a TZ ! example DateTime64(6,'UTC') becomes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it works if you use a String instead, CH will do the conversion Exception I got with the maximum boundaries
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is quite logical. We can take into account source.timezone for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes that works. @subkanthi FYI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you explain, please? I don't see any problem here (except possible slowness)). I specifically override it within There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the wrong place to change the schema. The sink-connector should create the schema with the correct timezone (target timezone). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the sink-connector, there are 2 places we fix the schema
what do you think ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in terms of timing, let us wait for @subkanthi to merge all pending PRs to develop, we will then address this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @subkanthi let us implement this too #349 |
||
Pattern pattern = Pattern.compile("DateTime(64)?(\\([^\\)]*\\))?"); | ||
Matcher matcher = pattern.matcher(typeName); | ||
StringBuffer result = new StringBuffer(""); | ||
int cursor = 0; | ||
while (matcher.find()) { | ||
int start = matcher.start(); | ||
int end = matcher.end(); | ||
result.append(typeName.substring(cursor, start)); | ||
cursor = end; | ||
String occurrence = typeName.substring(start, end); | ||
if (occurrence.contains("DateTime64")) { | ||
String[] params = StringUtils.substringBetween(occurrence, "(", ")").split(","); | ||
String precision = params[0].trim(); | ||
result.append(String.format("DateTime64(%s,\\'%s\\')", precision, timeZone)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. escape clashing with another escape There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in #366
|
||
} else { | ||
result.append(String.format("DateTime(\\'%s\\')", timeZone)); | ||
} | ||
} | ||
result.append(typeName.substring(cursor)); | ||
return result.toString(); | ||
} | ||
|
||
/** | ||
* Function to get the clickhouse version. | ||
* @return version as string. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure it is a good idea. leave it
null
by default and initialize with source timezone from Debezium.Add to the default config.yaml (commented)