feat: Align object array to collection serialization protocol v2#2218
feat: Align object array to collection serialization protocol v2#2218david1437 wants to merge 5 commits intoapache:mainfrom
Conversation
java/fury-core/src/main/java/org/apache/fury/serializer/ArraySerializers.java
Outdated
Show resolved
Hide resolved
|
Some CI tests are failing I am assuming its related to my implementation of the write / copy / read methods? Possibly not using try finally for push / pop generic types? |
java/fury-core/src/main/java/org/apache/fury/serializer/ArraySerializers.java
Show resolved
Hide resolved
| } | ||
| if (this.collectionSerializer != null) { | ||
| fury.getGenerics().pushGenericType(this.collectionGenericType); | ||
| ArrayAsList list = new ArrayAsList(0); |
There was a problem hiding this comment.
Could we cache this object as a field. Nested serialization is a rare case, if we cache it as a field, we could save object creation cost.
We could write like :
ArrayAsList list = this.list;
if (list == null) {
list = new ArrayAsList(0);
} else {
this.list = null
}
// before serialiazation
...
// after serialization
this.list = list
| this.collectionSerializer = new FuryArrayAsListSerializer(fury); | ||
| this.collectionGenericType = buildCollectionGenericType(dimension); | ||
| } else { | ||
| this.collectionSerializer = null; |
There was a problem hiding this comment.
Why it's null for 1d object array?
There was a problem hiding this comment.
I thought for 1D / Non nested generics that we intended to use the normal default serialization path that was in place before?
There was a problem hiding this comment.
That is only for primitive array. Other array still use Collection serialization protocol. List<String> and String[] should have same layout
There was a problem hiding this comment.
Ok I am under impression T[] can never have primitives only wrapper primitive classes should those use previous method as well ie. Double[] ?
|
Any suggestions on running the CI suites locally so I can work on debugging the issues? |
You can execute: cd java
mvn -T10 clean install -DskipTests
cd fury-core
mvn test -Dtest=org.apache.fury.serializer.ArraySerializersTest Or you can use IDE such jetbrains intellj to debug test one by one, I prefer this method: |
|
So I have been running the ArraySerializerTests and they have all been passing so a bit confused on why the CICD tests are failing |
|
@david1437 you could execute: cd java
mvn -T10 clean install -DskipTests
cd fury-core
mvn -T16 test`All failed tests will be printed in the end |
|
https://github.com/apache/fury/actions/runs/14971961752/job/42054796701?pr=2218 |
| value = this.collectionSerializer.read(buffer).toArray(); | ||
| fury.getGenerics().popGenericType(); | ||
| if (this.innerType.isPrimitive() || TypeUtils.isBoxed(this.innerType)) { | ||
| return Arrays.copyOf(value, value.length, this.type); |
There was a problem hiding this comment.
Considering this does a copy it does not feel good is there a better approach for handling this? without this i get a class cast exception?
|
After resolving the issues around primitives and depth increment it seems only a couple tests are failing, Would appreciate some help on those specific failing cases: JdkProxySerializerTest and NonexistentClassSerializersTest |
2 participants
What does this PR do?
Related issues
Does this PR introduce any user-facing change?
Benchmark