Initial commit of dfdlx:DefineCharset by jadams-tresys · Pull Request #1762 · apache/daffodil

jadams-tresys · 2026-10-20T18:22:13Z

Allows for a user to define a simple character set directly inside a DFDL schema, without requiring any plugins.

stevedlawrence

I really like this approach, this will definitely ease some pain points around custom characters sets. But I think there's room for simplification. Also some tests are failing, I think due to missing TDML support?

olabusayoT

Looks good...a few comments.

olabusayoT · 2026-10-30T20:41:05Z

+   * Demanding this value also demands validation (SDE checking) of every
+   * dfdlx:defineCharset statement, even those never referenced.


If they are not referenced, should we validate them? Wouldn't it be better to only validate those that are rederenced?

I think it's fine to validate all charsets defined in a schema, even if they aren't used in the schema being compiled. I'd want to know if I have an invalid charset in the schema so that way I can address it. I don't see any benefit in not checking it.

This is similar to how we will still throw an SDE for improperly defined elements, even if those elements aren't ever referenced in the root element of the schema being compiled.

stevedlawrence

+1 just some potential simplifications/minor improvements

stevedlawrence · 2026-11-06T17:29:58Z

+          Character used when unparsing a character that cannot be represented
+          in this charset. replacementChar must be a single character (DFDL
+          Character Entities can be used) and must be present in the
+          decodeString.


Thinking more about replacement char and some internal examples of this usage, what if we have something like this:

<dfdlx:defineCharset ... width=4" decodeString="ABCDEFGHIJ??????" replacementChar="?" />

So the encoding is 4 bits, but only supports 10 characters, and the remaining 6 are considered errors/question mark.

In this case, the replacementChar is going to use the index of the first ? character in the decode string, which is 0xA, but maybe a user wants to use 0xF instead. Using a character is very user friendly, but it also doesn't give user any control when the replacement character shows up multiple times in an encoding.

Maybe that's fine since ultimately 0xA and 0xF decode to the same value, but it does lose some degree of control.

Or if we do want to support it, maybe replacementChar does want to go back to being a number (e.g. replacementEncodeValue="15", not sure about that name though). If we do want to support this, I'd suggest we use a number instead of hex since DFDL doesn't really use hex syntax anywhere.

From a schema writer perspective, being able to just define it as a character seems easiest, instead of trying to figure out what index in the decode string your replacement character is defined as.

My concern is this isn't a made up example. We have charsets that do exactly this. For example, an unaligned ASCII charset is currently defined like this:

override lazy val decodeString = (0 to 127).map { _.toChar }.mkString + 255.toChar.toString * 128 override lazy val replacementCharCode = 0xff

So byte values 128 (0x80) to 255 (0xff) all map to the same 255 (0xff) codepoint, and the replacement character code is 255 (0xff). In Daffodil's current implementation unknown characters get unparsed to byte value 255 (0xff). But with defineCharset the first replacement character will be found at index 128, so we will unparse unknown characters to 128 (0x80). And there's no way with defineCharset to tell it use to 255 instead.

Maybe this is fine, and is just an acceptable limitation, but it does mean existing charsets cannot be changed to use this new feature without a change in behavior.

And it's not uncommon for a charset to have multiple values that aren't defined. For example, even US-ASCII and ISO-8859-1 both have byte values that have no codepoint mapping. These ones are maybe a bit different since there's an argument that an unknown value wants to unparse as ?, but unparsing to 0xff seems like a reasonable alternative that we don't currently support.

Fair enough, I'll update it to be replacementCodePointIndex and have it be an integer. That should make it clear that the codePoints attribute functions as a look up table as well.

Thoughts on just replacement? That's what java CharsetEncoder uses:

https://docs.oracle.com/javase/8/docs/api/java/nio/charset/CharsetEncoder.html#replacement--

replacementCodePointIndex feels very verbose. It is clear that it indexes into codepoint, but this property about unparse so doesn't even really need to have anything to do with the values in the codepoint. It just has be between 0 and the length of the codepoint.

olabusayoT

+1 typos and commented out code notwithstanding

stevedlawrence

+1, looks good, just a number of small things i noticed with fresh eyes

stevedlawrence · 2026-11-10T13:41:02Z

+    val str = getAttributeRequired("replacement")
+    val i = str.toIntOption
+    schemaDefinitionUnless(
+      (i.isDefined && (i.get >= 0 && i.get <= codePoints.length)),


I think this need to < codePoints.length instead of <=?

For example, an 8-bit width character set has 256 possible values, so codePoints.length is 256. But the largest value we can unparse as a replacement is 255 (0xff)

stevedlawrence · 2026-11-10T13:43:06Z

+    val i = str.toIntOption
+    schemaDefinitionUnless(
+      (i.isDefined && (i.get >= 0 && i.get <= codePoints.length)),
+      s"The replacement attribute must be defined as a an integer index in the codePoints string, provided value: $str"


Suggest we say "... must be between 0 and ${codePoints.length - 1}". Although our implementation requires it to be an index, that's kindof more of an implementation detail than an actual requirement.

stevedlawrence · 2026-11-10T13:45:26Z

      case Elem("dfdl", "defineEscapeScheme", _, _, _*) =>
        DFDLDefineEscapeSchemeFactory(node, this)
      case Elem("dfdl", "defineVariable", _, _, _*) => DFDLDefineVariable(node, this)
+      case Elem("dfdlx", "defineCharset", _, _, _*) => DFDLDefineCharset(node, this)


Just noticed this, should we change this to defineCharacterSet? DFDL always fully spells things out instead of using abbreviations. Internally I think it's fine to use things like DFDLDefineCharset but things that appear in the schema probably want to be be fully spelled out.

I decided to go ahead and rename everything to CharacterSet for consistency sake, as that is how the other properties are defined as well.

stevedlawrence · 2026-11-10T13:49:08Z

  }
+
+  private lazy val userDefinedCharsetsNameString =
+    userCharsets.keys.toSeq.sorted.mkString(", ")


This val is no longer used and can be removed.

stevedlawrence · 2026-11-10T13:52:08Z

+        <xs:documentation><![CDATA[
+          Index in the codePoints string of the replacement character. Used
+          when unparsing a character that cannot be represented in this
+          character set.


Suggest not mentioning index here. Same as another comment, it's kindof an implementation detail. I think it's more clear to just say "decimal value used to unparse characters that cannot be represented in this character set".

stevedlawrence · 2026-11-10T13:55:30Z

@@ -1753,7 +1759,12 @@ case class UnparserTestCase(ptc: NodeSeq, parentArg: DFDLTestSuite)
          if (actual.isScannable) {


I think this also needs the CharsetUtils.getCharset(..) != null check.

Might also suggest revertting the wrapping of verifyTextData above and below since that is no longer changed.

stevedlawrence · 2026-11-10T14:04:01Z

+  @Test def sixBitUser = test
+  @Test def replacement = test
+  @Test def ascii6BitUser = test
+  // @Test def largeCodePoints = test


I think the convention we've started moving to is using @Ignore to skip failing TDML tests.

Allows for a user to define a simple character set directly inside a DFDL schema, without requiring any plugins.

jadams-tresys requested review from olabusayoT and stevedlawrence October 20, 2026 18:22

stevedlawrence requested changes Oct 22, 2026

View reviewed changes

olabusayoT reviewed Oct 30, 2026

View reviewed changes

stevedlawrence reviewed Nov 6, 2026

View reviewed changes

olabusayoT approved these changes Nov 9, 2026

View reviewed changes

stevedlawrence approved these changes Nov 10, 2026

View reviewed changes

Implmenet dfdlx:DefineCharacterSet

6bcc5f7

Allows for a user to define a simple character set directly inside a DFDL schema, without requiring any plugins.

jadams-tresys force-pushed the DAFFODIL-2894 branch from 81d7111 to 6bcc5f7 Compare November 10, 2026 17:50

jadams-tresys merged commit 3535bac into apache:main Nov 10, 2026
10 of 11 checks passed

jadams-tresys deleted the DAFFODIL-2894 branch November 10, 2026 18:18

		* Demanding this value also demands validation (SDE checking) of every
		* dfdlx:defineCharset statement, even those never referenced.

		@@ -1753,7 +1759,12 @@ case class UnparserTestCase(ptc: NodeSeq, parentArg: DFDLTestSuite)
		if (actual.isScannable) {

Conversation

jadams-tresys commented Oct 20, 2026

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olabusayoT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

stevedlawrence Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olabusayoT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevedlawrence 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!

Uh oh!

Choose a reason for hiding this comment

stevedlawrence Nov 9, 2026 •

edited

Loading