MINOR: Refactor Values class to fix checkstyle, add benchmark, optimize exceptions

[gharris1727]

The Values class has checkstyle suppressions for NPathComplexity, MethodLength, CyclomaticComplexity, and JavaNCSS. Rather than suppressing these problems, we should refactor the oversize methods into smaller methods that satisfy the checkstyle heuristics.

I added a benchmark to verify that this refactoring does not cause a performance regression. Once I had the benchmark, I found some really obvious optimizations:

• parseAsTemporal was catching ParseExceptions, and I eliminated those by checking the ParsePosition.
• number parsing was catching multiple ArithmeticExceptions when numbers had decimal parts, or were too large to fit in certain types. The new implementation does some equivalent rounding and cast-checking to avoid the exceptions.

With these changes, the parse() profile shows a significant improvement, while all of the other methods are nearly the same or slightly faster than before. I'll include the detailed results from my machine in a follow-up comment.

Finally I just did some simplification that makes the generic convertTo method call the specific convertTo(Boolean,Byte, etc) methods, rather than having every one of the specific methods call the single generic method. This eliminates the need for casting Object to the return types, and eliminates flaws that could come from that cast failing. I fixed one such flaw in #15399 that I found while doing this refactor.

The SchemaMerger is a refactor of the fix in #15399 because the additional variables and control flow exceeded the checkstyle heuristics.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[gharris1727]

Benchmark	Before ns/op	Before Error	After ns/op	After Error	Speedup
ValuesBenchmark.testConvertToBoolean	124.038	0.319	71.448	2.258	1.7
ValuesBenchmark.testConvertToByte	111.404	2.196	61.334	0.092	1.8
ValuesBenchmark.testConvertToDate	3698.989	135.292	3675.007	5.379	1.0
ValuesBenchmark.testConvertToDecimal	1433.731	20.883	1515.147	2.91	0.9
ValuesBenchmark.testConvertToDouble	196.389	68.131	59.401	0.357	3.3
ValuesBenchmark.testConvertToFloat	329.249	158.87	56.547	0.43	5.8
ValuesBenchmark.testConvertToInteger	130.939	0.479	64.801	0.465	2.0
ValuesBenchmark.testConvertToList	1212.389	19.245	1527.234	2.949	0.8
ValuesBenchmark.testConvertToLong	131.72	2.178	75.345	0.434	1.7
ValuesBenchmark.testConvertToMap	1641.567	27.802	1890.402	12.093	0.9
ValuesBenchmark.testConvertToShort	113.9	0.254	61.587	0.199	1.8
ValuesBenchmark.testConvertToString	1620.263	8.395	1669.339	3.622	1.0
ValuesBenchmark.testConvertToStruct	3.768	0.034	1.374	0.013	2.7
ValuesBenchmark.testConvertToTime	2775.386	8.146	2685.46	8.681	1.0
ValuesBenchmark.testConvertToTimestamp	2849.605	7.025	2731.251	6.568	1.0
ValuesBenchmark.testInferSchema	116.758	0.133	97.536	0.325	1.2
ValuesBenchmark.testParseString	41501.659	197.842	13606.346	287.035	3.1

This has some irregularities. The floating point tests were multi-modal, but don't appear to be with the new implementation, i'm not sure why. The speedup numbers for those are a bit misleading.

# Benchmark: org.apache.kafka.jmh.connect.ValuesBenchmark.testConvertToFloat
# Warmup Iteration   1: 280.384 ns/op
# Warmup Iteration   2: 133.229 ns/op
# Warmup Iteration   3: 132.223 ns/op
Iteration   1: 430.475 ns/op
Iteration   2: 284.555 ns/op
Iteration   3: 273.401 ns/op
Iteration   4: 328.282 ns/op
Iteration   5: 237.622 ns/op
Iteration   6: 344.620 ns/op
Iteration   7: 405.791 ns/op

# Benchmark: org.apache.kafka.jmh.connect.ValuesBenchmark.testConvertToDouble
# Warmup Iteration   1: 200.209 ns/op
# Warmup Iteration   2: 138.782 ns/op
# Warmup Iteration   3: 195.215 ns/op
Iteration   1: 196.872 ns/op
Iteration   2: 192.461 ns/op
Iteration   3: 200.401 ns/op
Iteration   4: 216.845 ns/op
Iteration   5: 195.567 ns/op
Iteration   6: 137.116 ns/op
Iteration   7: 235.458 ns/op

[mimaison]

Thanks Greg for looking into this, this Values class was a bit of a monster. I made a couple of suggestions and this needs rebasing to resolve the checkstyle/suppressions.xml conflict.

[mimaison]

I wonder if it would make sense to move all these parse<>() methods to the Parser class, and extract Parser to its own file. WDYT?

I made a quick attempt in 10f4910#diff-024f49f1f6adf07bcc1cab6aa8caa0d931ba2c6be887d96ab575ae032be4d051

[gharris1727]

That's a cool idea, I think that makes a lot of sense when a bunch of these static methods take a Parser argument anyway.

Since this is in the public API, I'll focus on moving some of the internal methods to instance methods of a protected/package local Parser, and leave the public static methods in Values.

[mimaison]

Ah right, Parser is also part of the public API, I thought it was only a private inner class.

[gharris1727]

Parser was protected, so I think it's still safe to refactor. The class doesn't show up here: https://javadoc.io/doc/org.apache.kafka/connect-api/latest/index.html

I moved the existing Parser to Tokenizer, as it had a good interface already, and adding methods would just be clutter. The methods which took a Parser argument have now been moved to a single toplevel class named Parser. Both of these classes are package-local, so shouldn't appear in the API docs.

I left almost all of the private/protected static methods in Values, just bringing a few over that were only ever called by the Parser. I tried moving things from Values to Parser to break the circular dependency, but this required moving nearly everything to Parser. The two classes are really intertwined, and i'm not really satisfied with this refactor now.

[gharris1727]

My core difficulty is that the parsing logic and the conversion logic mutually depend on one another:

1. The convertTo methods check if the input is a String, and then run it through the Parser.
2. After parsing a map or array, the Parser calls convertTo on the elements to "cast" them to a common schema

I'm pretty sure convertTo -> parser -> convertTo is a reasonable cycle, and should happen all the time via convertToList, convertToMap.

I don't think that parser -> convertTo -> parser is a useful cycle for multiple reasons, but proving that is a little bit slippery. With some time I think I can break this part of the cycle so that this doesn't end up as one big ball of code again.

[mimaison]

The Parser class shows up in https://kafka.apache.org/37/javadoc/org/apache/kafka/connect/data/Values.Parser.html so not sure if we can make it private. I think the other inner classes are not really meant to be used by users. Should we do a small KIP to hide them all?

[mimaison]

Regarding the parser -> convertTo -> parser cycle, if we want to tackle it, it's definitively in another PR.

[gharris1727]

I'm confused. Locally for me this doesn't appear in the API docs, and it's because of this change: https://issues.apache.org/jira/browse/KAFKA-14839 . The ticket says it's fixed in 3.5.0, and that commit is definitely in 3.6.0+ so it should have applied when doing the 3.7.0 release.

I'm happy to preserve all of the protected methods (both the existing ones in Values, and the whole Parser class) if these are truly public API, but if this other commit already removed them from the Javadoc, I think we should try and follow through on the renaming here. I just need to figure out how they showed up in the 37 release docs...

[gharris1727]

Oh it's because there are two javadoc configs, and they got out-of-sync:
./gradlew javadoc uses this one:

kafka/build.gradle

Lines 597 to 610 in 5439914

	javadoc {
	options.charSet = 'UTF-8'
	options.docEncoding = 'UTF-8'
	options.encoding = 'UTF-8'
	options.memberLevel = JavadocMemberLevel.PUBLIC // Document only public members/API
	// Turn off doclint for now, see https://blog.joda.org/2014/02/turning-off-doclint-in-jdk-8-javadoc.html for rationale
	options.addStringOption('Xdoclint:none', '-quiet')
	// The URL structure was changed to include the locale after Java 8
	if (JavaVersion.current().isJava11Compatible())
	options.links "https://docs.oracle.com/en/java/javase/${JavaVersion.current().majorVersion}/docs/api/"
	else
	options.links "https://docs.oracle.com/javase/8/docs/api/"
	}

and ./gradlew aggregatedJavadoc uses this one:

kafka/build.gradle

Lines 3335 to 3353 in 5439914

	task aggregatedJavadoc(type: Javadoc, dependsOn: compileJava) {
	def projectsWithJavadoc = subprojects.findAll { it.javadoc.enabled }
	source = projectsWithJavadoc.collect { it.sourceSets.main.allJava }
	classpath = files(projectsWithJavadoc.collect { it.sourceSets.main.compileClasspath })
	includes = projectsWithJavadoc.collectMany { it.javadoc.getIncludes() }
	excludes = projectsWithJavadoc.collectMany { it.javadoc.getExcludes() }
	options.charSet = 'UTF-8'
	options.docEncoding = 'UTF-8'
	options.encoding = 'UTF-8'
	// Turn off doclint for now, see https://blog.joda.org/2014/02/turning-off-doclint-in-jdk-8-javadoc.html for rationale
	options.addStringOption('Xdoclint:none', '-quiet')
	// The URL structure was changed to include the locale after Java 8
	if (JavaVersion.current().isJava11Compatible())
	options.links "https://docs.oracle.com/en/java/javase/${JavaVersion.current().majorVersion}/docs/api/"
	else
	options.links "https://docs.oracle.com/javase/8/docs/api/"
	}

Apparently the javadoc website I linked uses the javadoc jar where the protected members are excluded, and the Kafka website uses the aggregatedJavadoc tree which still has the protected members.

[mimaison]

Good find! In that case I'm fine with the changes, no need for a KIP.

[mimaison]

A few of these convertTo<>() methods are not covered by unit tests. It's ok not to address this in this PR if you'd prefer as it's already huge.

[gharris1727]

I added tests to get the methods themselves up to 100% coverage, but the overall class still is missing some coverage. Thanks for pointing this out, as there were certainly some pretty obvious cases that weren't tested.

[gharris1727]

Here's the Values test coverage changes:

State	Class %	Method %	Line %
Initial	100% (4/4)	81% (40/49)	78% (464/589)
Added tests	100% (4/4)	97% (48/49)	85% (502/589)
Refactored	100% (6/6)	93% (77/82)	84% (565/669)

There are more classes, methods, and lines, but the percentage coverage went up.

[mimaison]

LGTM

[gharris1727]

Here's the final performance changes:

Benchmark	Before	Before Error	After	After Error	Speedup
ValuesBenchmark.testConvertToBoolean	123.749	0.842	72.577	0.412	1.7
ValuesBenchmark.testConvertToByte	117.883	1.397	62.387	0.148	1.9
ValuesBenchmark.testConvertToDate	3700.318	160.043	3522.214	36.075	1.1
ValuesBenchmark.testConvertToDecimal	1530.936	49.503	1485.654	11.766	1.0
ValuesBenchmark.testConvertToDouble	163.937	55.591	60.378	0.577	2.7
ValuesBenchmark.testConvertToFloat	204.591	109.567	57.611	0.49	3.6
ValuesBenchmark.testConvertToInteger	140.586	3.809	66.496	0.371	2.1
ValuesBenchmark.testConvertToList	1276.364	54.037	1601.568	28.972	0.8
ValuesBenchmark.testConvertToLong	132.029	3.118	76.744	1.112	1.7
ValuesBenchmark.testConvertToMap	1361.082	78.59	1244.339	11.019	1.1
ValuesBenchmark.testConvertToShort	121.575	4.6	63.167	0.311	1.9
ValuesBenchmark.testConvertToString	1667.243	51.186	1580.031	11.391	1.1
ValuesBenchmark.testConvertToStruct	3.819	0.082	1.395	0.009	2.7
ValuesBenchmark.testConvertToTime	2864.609	163.586	2701.721	60.677	1.1
ValuesBenchmark.testConvertToTimestamp	2789.008	30.371	2738.573	19.6	1.0
ValuesBenchmark.testInferSchema	123.196	3.292	99.336	0.867	1.2
ValuesBenchmark.testParseString	43826.599	922.077	13429.742	133.089	3.3

There's a consistent performance degradation for testConvertToList that seems to come from the parseString implementation being slightly less efficient for array inputs. I'll follow up on that separately since this PR already has too much scope, and the degradation is only slight.

One interesting final observation is that the variance/error for all of the tests is lower than the previous implementation. I suspect that this is because many of methods now avoid traversing the switch-case in the convertTo implementation, which lessens the number of branches and increases the branch predictor's success rate.

And here's the final coverage changes:

State	Class	Method	Line
Before	100% (4/4)	81% (40/49)	78% (464/589)
After	100% (6/6)	93% (77/82)	84% (565/669)