Add auto conversion option to convert_type processor by kkondaka · Pull Request #5782 · opensearch-project/data-prepper

kkondaka · 2025-06-16T17:53:46Z

Description

Add auto conversion option to convert_type processor. Adds the following config option to convert_type processor to automatically convert strings.

processor:
   - convert_type:
        <existing options>
        coerce_strings:
           time_formats : ["format1", "format2"] # optional

Issues Resolved

Resolves #5733

Check List

[ X] New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
[X ] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

san81 · 2025-06-16T19:15:54Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+        } else {
+            this.type = null;
+            this.converter = null;
+        }


nit: Probably else block is not needed if we assign null at their initialization.

I got error that variables are not initialized.

if null values are assigned at their initialization, it won't complain

san81 · 2025-06-16T19:24:07Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+        final LocalDate localDateForDefaultValues = LocalDate.now(DEFAULT_ZONE_ID);
+
+        final DateTimeFormatterBuilder dateTimeFormatterBuilder = new DateTimeFormatterBuilder()
+                .appendPattern(pattern)


What if the pattern already includes zone?
If possible, avoiding the use of DEFAULT_ZONE_ID will make it more flexible

san81 · 2025-06-16T19:42:13Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+            Object result = null;
+            try {
+                result = autoConvert(event, entry.getValue(), keyPrefix+entry.getKey()+"/");
+                if (result != null) {


if the value is null, do we want to still set null in that specific key? Looks like we are dropping that key here.

What do you mean? autoConvert only converts strings to boolean/integer/float/long/double. If the string is "null", it won't be converted to null, that's not something auto conversion currently handles.

san81 · 2025-06-16T19:46:30Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+                for (DateTimeFormatter formatter : coerceDateTimeFormatters) {
+                    try {
+                        ZonedDateTime tmp = ZonedDateTime.parse(str, formatter);
+                        long r = (long)tmp.toInstant().toEpochMilli();


Returning Instant type is better here, I guess

Why? We want long with milliseconds. How's Instant useful?

san81 · 2025-06-16T19:50:19Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+                return null;
+            } else if (lstr.contains(".") || lstr.contains("e")) {
+                Double d = Double.parseDouble(lstr);
+                if (d <= Float.MAX_VALUE && d >= Float.MIN_VALUE) {


nit: Probably keeping it as Double is better?

It looks like TargetType doesn't have Float. I guess, for consistency, I will remove Float.

san81 · 2025-06-16T19:55:51Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+                }
+                return d;
+            } else if (Character.isDigit(firstChar) || firstChar == '-' || firstChar == '+') {
+                Long l = Long.parseLong(str);


parseLong could throw exception. We may want to catch and ignore?

It is ignored in the caller of autoConvert

san81

Thank you for adding this enhancement. Just left a few comments.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

graytaylor0 · 2025-06-17T16:53:46Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+                if (result != null) {
+                    event.put(keyPrefix+entry.getKey(), result);
+                }
+            } catch (Exception ignored) {}


Might be worth having a metric here to debug if users have issues with using coerce_strings

coercion is best effort. Not sure if metric here makes sense. If there is a string starting with a digit like 1234-sdfjh-34 It will fail one or more of coercions but that's not necessarily metric worthy

graytaylor0 · 2025-06-17T16:56:01Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+                    }
+                }
+                return null;
+            } else if (lstr.contains(".") || lstr.contains("e")) {


Is it safe to assume this is double? Or if we try and fail it's just a no-op? What happens if parseDouble throws?

Again, It is a best effort. Float/doubles will always have one of both of these and they be converted properly. It is possible that a string like "sdfsdf.eeee" fits this condition and the code tries to convert and it will fail. And that's OK because it is a no-op. Yes, there is some wastage of cycles but there is no way to make sure a string cannot be coerced without actually parsing it. We are reducing unnecessary parse calls using this checks.

graytaylor0 · 2025-06-17T16:57:34Z

...java/org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessor.java

+            }
+
+        } else if (objValue instanceof Map) {
+            doAutoConversion(event, (Map<String, Object>)objValue, keyPrefix);


Should we put a limit on the recursion here?

Why? It will be finite, right?

graytaylor0 · 2025-06-17T16:58:20Z

...org/opensearch/dataprepper/plugins/processor/mutateevent/ConvertEntryTypeProcessorTests.java

+        String t3 = zonedDateTime.format(DateTimeFormatter.ofPattern(ConvertEntryTypeProcessorConfig.DEFAULT_TIME_STRING_FORMATS.get(2)));
+        String t4 = zonedDateTimePST.format(DateTimeFormatter.ofPattern(ConvertEntryTypeProcessorConfig.DEFAULT_TIME_STRING_FORMATS.get(3)));
+
+        final Map<String, Object> testData1 = new HashMap<>();


Might be good to add coverage for nested maps

There is a coverage for nested maps already. testData1 is inside testData

…ect#5782) * Add auto conversion option to convert_type processor Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Modified to coerse floats to double Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> Signed-off-by: Jonah Calvo <caljonah@amazon.com>

kkondaka requested review from KarstenSchnitter, chenqi0805, dinujoh, dlvenable, engechas, graytaylor0, oeyh, san81, sb2k16 and srikanthjg as code owners June 16, 2025 17:53

san81 reviewed Jun 16, 2025

View reviewed changes

kkondaka added 2 commits June 16, 2025 22:07

Add auto conversion option to convert_type processor

09c633b

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Addressed review comments

0167feb

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka force-pushed the auto-convert branch from 66d7ab3 to 0167feb Compare June 16, 2025 22:25

Modified to coerse floats to double

b8ae3df

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

san81 approved these changes Jun 16, 2025

View reviewed changes

graytaylor0 approved these changes Jun 17, 2025

View reviewed changes

kkondaka merged commit 69737c1 into opensearch-project:main Jun 17, 2025
46 of 47 checks passed

kkondaka added this to the v2.12 milestone Jun 24, 2025

kkondaka deleted the auto-convert branch July 1, 2025 17:04

Conversation

kkondaka commented Jun 16, 2025

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

san81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants