You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fory.apache.org by "lzaeh (via GitHub)" <gi...@apache.org> on 2025/10/05 12:36:13 UTC
[PR] fix(go): Initial adjustments to the list serializer [fory]
lzaeh opened a new pull request, #2703:
URL: https://github.com/apache/fory/pull/2703
<!--
**Thanks for contributing to Apache Fory™.**
**If this is your first time opening a PR on fory, you can refer to [CONTRIBUTING.md](https://github.com/apache/fory/blob/main/CONTRIBUTING.md).**
Contribution Checklist
- The **Apache Fory™** community has requirements on the naming of pr titles. You can also find instructions in [CONTRIBUTING.md](https://github.com/apache/fory/blob/main/CONTRIBUTING.md).
- Apache Fory™ has a strong focus on performance. If the PR you submit will have an impact on performance, please benchmark it first and provide the benchmark result here.
-->
## Why?
In Go, the list serializer previously lacked certain fast-path matches and didn’t handle some nested cases well during deserialization; this PR fixes part of those issues.
<!-- Describe the purpose of this PR. -->
## What does this PR do?
### 1. It enhances the slice serializer in Go, mainly by:
- aligning the protocol handling steps;
- adding the fast path for homogeneous cases;
- restoring element types in []interface{} as accurately as possible during deserialization. Specifically:
- For struct, array, and slice: []T, we can faithfully recover the inner element types on receipt;
- For map contained in []interface{}, we can only receive it as map[interface{}]interface{} for now; I haven’t found a better approach yet.
### 2. Primary function/file changes:
- To accommodate scenarios where needWriteRef requires a “pending” state rather than a strict boolean, I followed Python and changed it to int8, using 1 for need, 0 for not-need, and -1 for pending.
- Added a type parameter to getTypeInfo so that we can query typeinfo in situations where only a type is available; forcing the target to carry a value would be unnecessarily bloated in these cases.
- I moved the array serializer into array.go. I’m open to feedback on whether this is appropriate. The array serializer changes focus on:
a. enabling []T to receive [n]T;
b. due to Go’s type registration constraints, resource metadata appears to be registered via the slice type. Therefore, when serializing arrays we convert them to the corresponding slice type to fetch resources; during deserialization, read reconstructs an array as the receiver.
### 3. Known gaps / not yet addressed:
- Zero-copy and out-of-band memory tests are failing; I’ll continue studying and iterating on them.
- From the docs, multi-dimensional arrays of primitive types, like [x][y][z]primitiveTypes, are categorized as tensors. The current Go side treats them as slices, which is out of spec and needs attention.
- Some fast paths in the list serializer (Go’s slice serializer), especially those related to int, are not fully understood. I only read the Python code and implemented parts based on assumptions, which is not rigorous and is problematic. I will read the Java code and documentation to clarify.
- I didn’t handle issue #2639. The id removal there is tied to the registration refactor. I was informed that a teammate has already completed that refactor, so I left it untouched for now.
### 4. Newly discovered issue (flag semantics diverge between Java and Python for the list serializer):
- In Python, the flags are: COLLECTION_NOT_DECL_ELEMENT_TYPE = 0b100 and COLLECTION_NOT_SAME_TYPE = 0b1000
In Java, they are: IS_DECL_ELEMENT_TYPE = 0b100 and IS_SAME_TYPE = 0b1000
- The bit values are identical, but the semantics are inverted. Initially I suspected the code matched them inversely, until I created a List<String> in Java, serialized it, copied the bytes verbatim into Python, and attempted cross-language deserialization. It failed for the following reasons:
- Under Java semantics, serializing List<String> is considered “declared and same type.” In my test the final list flag was 8; the deserialization debug trace also showed it being treated as declared and same type.
<img width="871" height="528" alt="image" src="https://github.com/user-attachments/assets/2b29c6a9-7e88-4829-a34a-9f7ebdd57d4b" />
- In Python, it reads the same flag = 8, but due to different semantics and flow, Python interprets it as “non-declarative.” Because it is treated as non-declarative, Python proceeds to read the element type ID from the byte stream; however, declarative + homogeneous path does not write element type IDs, which causes byte misalignment.
<img width="879" height="132" alt="image" src="https://github.com/user-attachments/assets/464fe426-fd2a-49b9-b35c-d5acc7e8a614" />
The aforementioned naive test is as follows:
<img width="591" height="371" alt="image" src="https://github.com/user-attachments/assets/4bae07a9-1555-48c9-a1bd-acc5e2845cd5" />
<img width="846" height="151" alt="image" src="https://github.com/user-attachments/assets/ee8c707b-ac55-4d70-90be-d047b1f19be7" />
I’m wondering whether there’s a reason I’ve overlooked, whether my usage is incorrect, and how this should be handled properly.
### 5. Questions and concerns:
- To pass cross-language tests, I currently aligned with Python semantics. But small Java-side tests surfaced the issue above. This looks like a semantic mismatch in flags, yet it effectively means that when serializing the same list<T>, one side treats it as declarative while the other as non-declarative. Because of this, I temporarily skipped one batch test for slices locally. Although I aligned with Python semantics in implementation, my personal view is: for one-dimensional slices whose element type T is not map, they can generally be treated as declarative constructions, and the receiver should restore elements with accurate types where possible. I’m not sure if this is correct.
- If that understanding is incorrect or incomplete, I’m curious about the exact conditions under which a “declarative construction” of list is triggered (I may have missed details in the documentation).
- map is particularly tricky. Both slice and map have unique IDs but may vary in concrete types in Go. When []interface{} contains such values, deserialization becomes challenging. Given the slice design, I added some admittedly inelegant code (with ample room for refactoring) to accurately receive various []T where T ≠ map inside []interface{}. However, if []interface{} contains a map, I still haven’t found a way to place it into a precisely-typed container; the fallback is to use map[interface{}]interface{} and rely on user-side handling. I’m not sure whether this approach is acceptable.
I’d like this to be a draft PR; I’ll work on fixing the remaining two or three tests and continue exploring how to refactor the code introduced in this PR.
<!-- Describe the details of this PR. -->
## Related issues
- #2633
<!--
Is there any related issue? If this PR closes them you say say fix/closes:
- #xxxx0
- #xxxx1
- Fixes #xxxx2
-->
## Does this PR introduce any user-facing change?
<!--
If any user-facing interface changes, please [open an issue](https://github.com/apache/fory/issues/new/choose) describing the need to do so and update the document if necessary.
Delete section if not applicable.
-->
- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
## Benchmark
<!--
When the PR has an impact on performance (if you don't know whether the PR will have an impact on performance, you can submit the PR first, and if it will have impact on performance, the code reviewer will explain it), be sure to attach a benchmark data here.
Delete section if not applicable.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "pandalee99 (via GitHub)" <gi...@apache.org>.
pandalee99 commented on code in PR #2703:
URL: https://github.com/apache/fory/pull/2703#discussion_r2404471535
##########
go/fory/fory_test.go:
##########
@@ -573,32 +574,6 @@ func TestMapEachIndividually(t *testing.T) {
}
}
-func TestArrayEachIndividually(t *testing.T) {
- fory := NewFory(true)
- for _, srcAny := range commonArray() {
- srcType := reflect.TypeOf(srcAny)
- endPtr := reflect.New(srcType)
- data, _ := fory.Marshal(srcAny)
- _ = fory.Unmarshal(data, endPtr.Interface())
- endVal := endPtr.Elem()
- endAny := endVal.Interface()
- require.Equal(t, srcAny, endAny)
- }
-}
-
-func TestSliceEachIndividually(t *testing.T) {
Review Comment:
This test code seems to have remained unchanged. Should we delete it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "lzaeh (via GitHub)" <gi...@apache.org>.
lzaeh commented on PR #2703:
URL: https://github.com/apache/fory/pull/2703#issuecomment-3375410748
> > @junjiexh Will there be conflicts among these PR? It seems that I have noticed some PR also want to try to solve this problem.
> > Maybe there are some logical functions that we need to align. Do you think he's doing that right? @chaokunyang
>
> this pr(#2656) revert the meaning of bit 0b100(from CollectionNotDeclElementType to CollectionIsDeclElementType) and some else.
sorry, I didnt pull the newest code and learn the protocol from a wrong logic python code, now i found it has been updated and aligned with java version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "junjiexh (via GitHub)" <gi...@apache.org>.
junjiexh commented on PR #2703:
URL: https://github.com/apache/fory/pull/2703#issuecomment-3369928265
> @junjiexh Will there be conflicts among these PR? It seems that I have noticed some PR also want to try to solve this problem.
>
> Maybe there are some logical functions that we need to align. Do you think he's doing that right? @chaokunyang
this pr(https://github.com/apache/fory/pull/2656) revert the meaning of bit 0b100(from CollectionNotDeclElementType to CollectionIsDeclElementType) and some else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "lzaeh (via GitHub)" <gi...@apache.org>.
lzaeh closed pull request #2703: fix(go): Initial adjustments to the list serializer
URL: https://github.com/apache/fory/pull/2703
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "lzaeh (via GitHub)" <gi...@apache.org>.
lzaeh commented on PR #2703:
URL: https://github.com/apache/fory/pull/2703#issuecomment-3369059375
Sorry — this PR is riddled with mistakes, and I’m honestly pretty disoriented. Please close it so it doesn’t introduce anything misleading. In some parts I was even talking nonsense with screenshots right in front of me. I may have seriously misunderstood the protocol. Please give me some time for self-critique and adjustment. @chaokunyang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "pandalee99 (via GitHub)" <gi...@apache.org>.
pandalee99 commented on code in PR #2703:
URL: https://github.com/apache/fory/pull/2703#discussion_r2404470998
##########
go/fory/fory_test.go:
##########
@@ -158,10 +158,11 @@ func TestSerializePtr(t *testing.T) {
}
func TestSerializeSlice(t *testing.T) {
+ t.Skip()
Review Comment:
Should this test be skipped?
##########
go/fory/map.go:
##########
@@ -121,7 +121,7 @@ func (s mapSerializer) Write(f *Fory, buf *ByteBuffer, value reflect.Value) erro
} else {
if valValid {
if valueSerializer != nil {
- if valueSerializer.NeedWriteRef() {
+ if valueSerializer.NeedWriteRef() == 1 {
Review Comment:
This way of judgment seems not elegant enough. @chaokunyang Do you have any suggestions?
##########
go/fory/fory_test.go:
##########
@@ -573,32 +574,6 @@ func TestMapEachIndividually(t *testing.T) {
}
}
-func TestArrayEachIndividually(t *testing.T) {
- fory := NewFory(true)
- for _, srcAny := range commonArray() {
- srcType := reflect.TypeOf(srcAny)
- endPtr := reflect.New(srcType)
- data, _ := fory.Marshal(srcAny)
- _ = fory.Unmarshal(data, endPtr.Interface())
- endVal := endPtr.Elem()
- endAny := endVal.Interface()
- require.Equal(t, srcAny, endAny)
- }
-}
-
-func TestSliceEachIndividually(t *testing.T) {
Review Comment:
This test code seems to have remained unchanged. Should we delete it?
##########
go/fory/fory_test.go:
##########
@@ -573,32 +574,6 @@ func TestMapEachIndividually(t *testing.T) {
}
}
-func TestArrayEachIndividually(t *testing.T) {
- fory := NewFory(true)
- for _, srcAny := range commonArray() {
- srcType := reflect.TypeOf(srcAny)
- endPtr := reflect.New(srcType)
- data, _ := fory.Marshal(srcAny)
- _ = fory.Unmarshal(data, endPtr.Interface())
- endVal := endPtr.Elem()
- endAny := endVal.Interface()
- require.Equal(t, srcAny, endAny)
- }
-}
-
-func TestSliceEachIndividually(t *testing.T) {
Review Comment:
This test code seems to have remained unchanged. Should we delete it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(go): Initial adjustments to the list serializer [fory]
Posted by "pandalee99 (via GitHub)" <gi...@apache.org>.
pandalee99 commented on PR #2703:
URL: https://github.com/apache/fory/pull/2703#issuecomment-3369036574
@junjiexh Will there be conflicts among these PR?
It seems that I have noticed some PR also want to try to solve this problem.
Maybe there are some logical functions that we need to align.
Do you think he's doing that right? @chaokunyang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org