Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(javascript): Added serialization and serialization for Type Meta Layer #1825

Merged
merged 35 commits into from
Sep 28, 2024

Conversation

Forchapeatl
Copy link
Contributor

@Forchapeatl Forchapeatl commented Sep 3, 2024

What does this PR do?

  • This PR is focused on registering Meta Types for the serialization and deserialization of Meta Layers.
  • By registering Meta Types correctly, the Fury framework can manage and process objects that require detailed metadata, further enhancing the interoperability between different systems using the Fury framework.

Related issues

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

The TypeScript file Before :

image

The Result as JavaScript:
image

@@ -44,6 +48,8 @@ export default class {
this.referenceResolver = new ReferenceResolver(this.binaryReader);
this.classResolver.init(this);
this.anySerializer = new AnySerializer(this);
this.typeMeta = new TypeMeta(BigInt(0),[]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_bytes in TypeMeta is a static method, if you want to call it, you should replace by this.typeMeta = TypeMeta

@Forchapeatl
Copy link
Contributor Author

Hi @theweipeng on line 147 , when I do this

-    ${this.builder.typeMeta.from_bytes(this.builder.reader.ownName())}
+   ${this.builder.reader.buffer(buffer.byteLength)}

I pass 3 test
Screenshot from 2024-09-14 09-25-13

Please for a hint on what I am missing.

@theweipeng
Copy link
Member

Hi @theweipeng on line 147 , when I do this

-    ${this.builder.typeMeta.from_bytes(this.builder.reader.ownName())}

+   ${this.builder.reader.buffer(buffer.byteLength)}

I pass 3 test

Screenshot from 2024-09-14 09-25-13

Please for a hint on what I am missing.

Why did you remove reading the typeMeta? I am confused

@Forchapeatl
Copy link
Contributor Author

Forchapeatl commented Sep 19, 2024

Why did you remove reading the typeMeta? I am confused

@theweipeng I was just experimenting . After solving the syntax error, I was trying to figure out a way to pass the tests.

@theweipeng
Copy link
Member

Hi @theweipeng on line 147 , when I do this

-    ${this.builder.typeMeta.from_bytes(this.builder.reader.ownName())}

+   ${this.builder.reader.buffer(buffer.byteLength)}

I pass 3 test
Screenshot from 2024-09-14 09-25-13
Please for a hint on what I am missing.

Why did you remove reading the typeMeta? I am confused

Ensure that the byte length consumed from the reader matches the length written to the buffer in the writeStmt. In the context of your code, it should be expressed as ${this.builder.reader.buffer(buffer2.byteLength)}

@Forchapeatl Forchapeatl changed the title Forchapeatl type meta feat(javascriot) Added serialization and serialization for Type Meta Layer Sep 24, 2024
@Forchapeatl Forchapeatl changed the title feat(javascriot) Added serialization and serialization for Type Meta Layer feat(javascript) Added serialization and serialization for Type Meta Layer Sep 24, 2024
@Forchapeatl Forchapeatl changed the title feat(javascript) Added serialization and serialization for Type Meta Layer feat(javascript): Added serialization and serialization for Type Meta Layer Sep 24, 2024
@Forchapeatl Forchapeatl marked this pull request as ready for review September 24, 2024 20:26
@Forchapeatl
Copy link
Contributor Author

@theweipeng , please have a look at my PR

@theweipeng
Copy link
Member

Can you add a config key to Config when constructing fury,

enum Mode {
    SchemaConsistent,
    Compatible,
}

new Fury({
    mode: Mode
})

And only write/read typeMeta when the mode is Compatible.

@Forchapeatl
Copy link
Contributor Author

@theweipeng
Copy link
Member

Hello @theweipeng , please is this the right place to add it.

https://github.com/apache/fury/blob/main/javascript/packages/fury/lib/fury.ts#L36

Yes

@Forchapeatl
Copy link
Contributor Author

I seem to have arrived at new errors. My I know the default of fury Mode ?

@theweipeng
Copy link
Member

I seem to have arrived at new errors. My I know the default of fury Mode ?

default mode should be consistenceMode not compatiableMode

Copy link
Member

@theweipeng theweipeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@theweipeng theweipeng merged commit ca4f2ac into apache:main Sep 28, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants