[HUDI-4365] Fixing URL-encoding in Bulk Insert row-writing path #6049
Conversation
|
@hudi-bot run azure |
| boolean hiveStylePartitioningEnabled = writeConfig.isHiveStylePartitioningEnabled(); | ||
|
|
||
| partitionPath = KeyGenUtils.handlePartitionPathDecoration(partitionPathField, | ||
| partitionPathValue == null ? null : partitionPathValue.toString(), shouldURLEncodePartitionPath, hiveStylePartitioningEnabled); |
There was a problem hiding this comment.
Consider passing Option.of(partitionPathValue) instead of null
There was a problem hiding this comment.
Yeah, done that initially but then decided to optimize it out given that this is a hot-path
| if (shouldURLEncodePartitionPath || isHiveStylePartitioned) { | ||
| sqlContext.udf().register( | ||
| partitionPathDecorationUDFName, | ||
| (UDF1<String, String>) partitionPathValue -> |
There was a problem hiding this comment.
So, I assume this UDF registration would be gone after HUDI-3993?
| public static final String FILE_SCHEME = "file"; | ||
| public static final String COLON = ":"; | ||
| public static final Random RANDOM = new Random(); | ||
| public static final Random RANDOM = new Random(0xDEED); |
…path as well; Cleaned up `BulkInsertDataInternalWriterHelper` to leverage the same sequence for partition-path handling
alexeykudinkin
force-pushed
the
ak/blk-ins-url-enc-fix
branch
from
48dc9f1 to
06d50fd
Compare
|
@hudi-bot run azure |
alexeykudinkin
force-pushed
the
ak/blk-ins-url-enc-fix
branch
from
06d50fd to
be896fe
Compare
| DataTypes.StringType); | ||
|
|
||
| rowDatasetWithRecordKeysAndPartitionPath = | ||
| rows.withColumn(HoodieRecord.RECORD_KEY_METADATA_FIELD, functions.col(recordKeyFields).cast(DataTypes.StringType)) |
There was a problem hiding this comment.
In ComplexKeyGenerator.getRecordKey(Row row), we setup prefixFieldName as true in method RowKeyGeneratorHelper.getRecordKeyFromRow(row, getRecordKeyFields(), recordKeySchemaInfo, true), so the record key will have a prefix, which is record key field name, when we use ComplexKeyGenerator.
As I understand here, we use withColumn here for recordKeyFields, then we will get the same value in RECORD_KEY_METADATA_FIELD as the original recordKeyFields, so no prefix when key generator is ComplexKeyGenerator. Will it cause problem?
There was a problem hiding this comment.
@TengHuo That's a good point. It can be a problem if you mix the write operation type or switch row-writing config for a table. I would suggest filing another JIRA ticket to keep it consistent across. I don't deem it to be a blocker but would be good to keep it consistent.
There was a problem hiding this comment.
okay, got it
then think it will have duplicate data issue if user upgrade from 0.10 or older version when they only setup one column as record key and use ComplexKeyGenerator . The same issue as upgrading from 0.10 to 0.11.
codope
left a comment
codope
left a comment
There was a problem hiding this comment.
LGTM
@alexeykudinkin Can you please rebase? CI instability was fixed recently.
|
Closing in favor of #5523 |
4 participants
Tips
What is the purpose of the pull request
Currently when doing bulk-insert using partition paths with slashes in it's being laid out incorrectly missing URL-encoding for the partition path, even though it's set to true.
This fix is purely a duct-tape until it's properly addressed by HUDI-3993
Brief change log
See above
Verify this pull request
This pull request is already covered by existing tests, such as (please describe tests).
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.