feat(QTDI-1305): improve error in record #1041

yyin-talend · 2025-05-07T08:09:35Z

Requirements

Any code change adding any logic MUST be tested through a unit test executed with the default build
Any API addition MUST be done with a documentation update if relevant

Why this PR is needed?

What does this PR adds (design/code thoughts)?

sonar-eks · 2025-05-09T05:43:45Z

78.80% Coverage on New Code (is less than 80.00%)
4.56% Duplicated Lines (%) on New Code (is greater than 3.00%)

Analysis Details

7 Issues

0 Bugs
0 Vulnerabilities
7 Code Smells

Coverage and Duplications

78.80% Coverage (56.40% Estimated after merge)
4.56% Duplicated Code (1.40% Estimated after merge)

Project ID: org.talend.sdk.component:component-runtime

View in SonarQube

undx

As said in comments, I think that errors should be handled at record level not schema one.
The workflow expects that checking for bad values is done outside the framework.
Maybe, we could handle rather in it?

Another lead may be :

define rules for bad data handling in schema
use the usual withType in Record. (checking is done here as for null values handling).
wdyt?

undx · 2025-05-09T08:16:24Z

component-api/src/main/java/org/talend/sdk/component/api/record/Record.java

@@ -34,6 +34,8 @@

 public interface Record {

+    String RECORD_ERROR_SUPPORT = "talend.sdk.runtime.record.error.support";


Usage in framework is to use talend.component. as a prefix for system properties.

undx · 2025-05-09T08:17:43Z

component-api/src/main/java/org/talend/sdk/component/api/record/Record.java

@@ -311,6 +313,11 @@ default Optional<Record> getOptionalRecord(final String name) {
        return ofNullable(get(Record.class, name));
    }

+    default boolean isValid() {
+        return !getSchema().getAllEntries()
+                .anyMatch(entry -> !entry.isValid());


See sonar comment, more easy to read that double negation.

undx · 2025-05-09T08:30:03Z

component-api/src/main/java/org/talend/sdk/component/api/record/Schema.java

+        /**
+          * @return true if the value of this entry is valid; false for invalid value.
+         */
+        boolean isValid();


Can also add a default behavior as returning true...

It is not using schema, but using Entry.

undx · 2025-05-09T08:28:53Z

component-api/src/main/java/org/talend/sdk/component/api/record/SchemaProperty.java

+    String ENTRY_IS_ON_ERROR = "entry.on.error";
+
+    String ENTRY_ERROR_MESSAGE = "entry.error.message";
+
+    String ENTRY_ERROR_FALLBACK_VALUE = "entry.error.fallback.value";
+
+    String ERROR_EXCEPTION = "entry.error.exception";


see my previous comment. Maybe smtg like record.value.

undx · 2025-05-09T08:30:48Z

component-api/src/main/java/org/talend/sdk/component/api/service/schema/Schema.java

@@ -163,6 +163,10 @@ public String getProp(final String property) {
        public Builder toBuilder() {
            throw new UnsupportedOperationException("#toBuilder()");
        }
+
+        @Override
+        public boolean isValid() { return true; }


See previous comments...

undx · 2025-05-09T08:32:59Z

...am/src/test/java/org/talend/sdk/component/runtime/beam/spi/record/AvroRecordBuilderTest.java

+        assertNotNull(entry2);
+        Assertions.assertFalse(entry2.isValid());
+
+        System.setProperty(Record.RECORD_ERROR_SUPPORT, "false");


You can use the before and after annotations to handle this...

undx · 2025-05-09T08:48:41Z

...r/src/main/java/org/talend/sdk/component/sample/feature/supporterror/SupportErrorMapper.java

+
+    @Emitter
+    public SupportErrorInput createSource() {
+


empty line, applied formater?

undx · 2025-05-09T08:49:54Z

...or/src/main/java/org/talend/sdk/component/sample/feature/supporterror/SupportErrorInput.java

+    @GridLayout(value = {
+            @GridLayout.Row("dataset"),
+    })
+     @Version


indentation

undx · 2025-05-09T08:59:04Z

...am/src/test/java/org/talend/sdk/component/runtime/beam/spi/record/AvroRecordBuilderTest.java

+        Schema customerSchema = schemaBuilder.withEntry(nameEntry).withEntry(nmEntry).withEntry(ageEntry).build();
+        // record 1
+        Record.Builder recordBuilder = factory.newRecordBuilder(customerSchema);
+        Record record1 = recordBuilder.withError("name", null, "Stirng is null", null)
+                .withString("normal", "normal")
+                .withError("age", "string", "is not an int", null)
+                .build();
+        assertFalse(record1.isValid());
+
+        final Schema.Entry entry =
+                record1.getSchema().getEntries().stream().filter(e -> "name".equals(e.getName())).findAny().get();
+        assertNotNull(entry);
+        Assertions.assertFalse(entry.isValid());
+
+        final Schema.Entry entry2 =
+                record1.getSchema().getEntries().stream().filter(e -> "age".equals(e.getName())).findAny().get();
+        assertNotNull(entry2);
+        Assertions.assertFalse(entry2.isValid());


Here, we're testing the Record/Schema implementations, not the Avro related.

undx · 2025-05-09T09:02:31Z

component-runtime-impl/src/main/java/org/talend/sdk/component/runtime/record/RecordImpl.java

+                // duplicate the schema instance with a modified Entry
+                final Entry oldEntry = this.findExistingEntry(columnName);
+                final Entry updateEntry = oldEntry.toBuilder()
+                        .withName(columnName)
+                        .withNullable(true)
+                        .withType(oldEntry.getType())
+                        .withProp(SchemaProperty.ENTRY_IS_ON_ERROR, "true")
+                        .withProp(SchemaProperty.ENTRY_ERROR_MESSAGE, errorMessage)
+                        .withProp(SchemaProperty.ENTRY_ERROR_FALLBACK_VALUE, String.valueOf(value))
+                        .withProp(SchemaProperty.ERROR_EXCEPTION, exception == null ? "" : exception.toString())
+                        .build();
+                return updateEntryByName(columnName, updateEntry);


This intent expects that value is always in error.

undx · 2025-05-09T14:34:45Z

Discussed with Yves and forget my comments on Schema level vs Record Level.
Removing my comments for clarity.

yyin-talend and others added 18 commits April 21, 2025 10:23

Add withError in Record.Builder

c924108

Test for AvroRecord

ae4cad7

fix isValid

9205765

Merge branch 'master' into yyin/QTDI-1305-ImproveErrorInRecord

cfa68e8

Add sample-feature\supporterror

cf0a3c6

worked

7d049d1

fix bug

441faae

Merge branch 'master' into yyin/QTDI-1305-ImproveErrorInRecord

0d8f532

Add new method to white list.

de71297

Add "valid":true to serialization

d5ab539

Add "valid":true to serialization

86ad7ee

Add "valid":true to serialization

884420f

rename

a9450e3

revert junit which no need to change

727c512

fix one problem

e2dbb27

Merge branch 'master' into yyin/QTDI-1305-ImproveErrorInRecord

21cccfc

fix import order

d2cf309

fix import order

02a5ebd

yyin-talend changed the title ~~Yyin/QTDI-1305 improve error in record~~ feat(QTDI-1305): improve error in record May 7, 2025

change command jar

92df0c0

This comment has been minimized.

Sign in to view

fix sonar

fb6a891

yyin-talend requested a review from undx May 8, 2025 07:39

remove comment

7314bfb

This comment has been minimized.

Sign in to view

add readme

01389f5

undx reviewed May 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(QTDI-1305): improve error in record #1041

feat(QTDI-1305): improve error in record #1041

yyin-talend commented May 7, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sonar-eks bot commented May 9, 2025

undx left a comment

undx May 9, 2025

undx May 9, 2025

undx May 9, 2025

yyin-talend May 9, 2025

undx May 9, 2025 •

edited

Loading

undx May 9, 2025

undx May 9, 2025

undx May 9, 2025

undx May 9, 2025

undx May 9, 2025

undx May 9, 2025

undx commented May 9, 2025

		@@ -34,6 +34,8 @@

		public interface Record {

		String RECORD_ERROR_SUPPORT = "talend.sdk.runtime.record.error.support";

feat(QTDI-1305): improve error in record #1041

Are you sure you want to change the base?

feat(QTDI-1305): improve error in record #1041

Conversation

yyin-talend commented May 7, 2025

Requirements

Why this PR is needed?

What does this PR adds (design/code thoughts)?

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sonar-eks bot commented May 9, 2025

Analysis Details

7 Issues

Coverage and Duplications

undx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

undx May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

undx commented May 9, 2025

undx May 9, 2025 •

edited

Loading