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

[WIP] Updating to On Demand #1

Closed
wants to merge 27 commits into from

Conversation

NicolasJiaxin
Copy link
Collaborator

I have begun to update RcppSimdJson to On Demand. The main complication that I have seen is that On Demand does not have specific JSON type for numbers. It labels double, signed integers and unsigned integers as json_type::number. To work around this, I have created a enum class complete_json_type that can identify the type of a json_type::number. This might not be an optimal solution, but it can serve as a prototype. I have not yet worked on matrix and data frame deserialization, only on scalar and vector deserialization, but there might still be typos and broken things in those files.

@NicolasJiaxin
Copy link
Collaborator Author

Also, I am not sure on how to run the tests.

@NicolasJiaxin
Copy link
Collaborator Author

Ping @lemire

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

@NicolasJiaxin I had to approve the tests, it seems, but now they run (with failures).

@NicolasJiaxin
Copy link
Collaborator Author

Ok thank you! At least now I can see what I have to fix

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

The main complication that I have seen is that On Demand does not have specific JSON type for numbers. It labels double, signed integers and unsigned integers as json_type::number.

I think that a reasonable default is just to parse everything as 'double' which is what you would expect from a JSON output.

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

I think that a reasonable default is just to parse everything as 'double' which is what you would expect from a JSON output.

But that is maybe something we can smooth over.

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

Though for now, I suggest you just treat all numbers as 'double'.

@NicolasJiaxin
Copy link
Collaborator Author

I will try that.

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

Interesting. It appears that I have to approve each run. Hmmm... :-)

@NicolasJiaxin
Copy link
Collaborator Author

This will probably not work. It need more cleaning..

@NicolasJiaxin
Copy link
Collaborator Author

I will push here when I think I have a good version.

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

I will push here when I think I have a good version.

It is fine. I do not mind approving the tests... I just do not understand why it does that.

@lemire
Copy link
Owner

lemire commented Jul 30, 2021

@NicolasJiaxin I gave you access to this repo., so maybe the tests will run automatically?

@NicolasJiaxin
Copy link
Collaborator Author

I think I am going to start over from a fresh branch. I think I have deleted too much stuff that it is causing me problem.. Should not take too long now that I have a clear idea of what to do.

@lemire
Copy link
Owner

lemire commented Aug 5, 2021

Great!

@lemire
Copy link
Owner

lemire commented Aug 9, 2021

Closing.

@lemire lemire closed this Aug 9, 2021
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