Improved table encoding code to handle sparse arrays. #28

aryajur · 2020-07-19T22:33:24Z

By going through the table before to get the maximum index we can handle sparse arrays. Also tostring seems to work better by maintaining float and integer number conversion.

…numbers are encoding improved.

Mysak0CZ · 2020-08-18T08:53:57Z

Hello! This would be nice change, but table.pack() creates an array with property n. Would it be possible to add check to line 71, that if k == "n" and type(v) == "number" then length = v else treat as object?
Lua documentation: https://www.lua.org/manual/5.2/manual.html#pdf-table.pack
Current behaviour:

> json.encode({1, nil, 3})
"[1,null,3]"
> json.encode(table.pack(1, nil, 3))
"{1:1,3:3,\"n\":3}"

Why is this beneficial?
If lua array ends with nil(s), they are trimmed. If we however use table.pack, the original count of objects is stored in property n allowing the array to end with nil/null.

… The array detection code now allows for the array to have a key n with a number value to detect the array length.

aryajur · 2020-08-19T04:43:52Z

Fixed that. Now arrays can be specified through table.pack.

appgurueu · 2022-12-17T16:10:44Z

json.lua

@@ -108,7 +119,7 @@ local function encode_number(val)
  if val ~= val or val <= -math.huge or val >= math.huge then
    error("unexpected number value '" .. tostring(val) .. "'")
  end
-  return string.format("%.14g", val)
+  return tostring(val)


Unreliable.

Don't understand. Perhaps a more useful/productive comment? Thanks.

Sorry, that was indeed too short to be useful. The documentation of tostring says:

Receives an argument of any type and converts it to a string in a reasonable format. For complete control of how numbers are converted, use string.format.

It doesn't guarantee anything. It could output the number with arbitrarily poor precision, or it could use an arbitrary format (which need not be valid JSON). (In practice, it does indeed behave quite reasonable, but you shouldn't rely on this; it should also be noted that it is not "lossless" in practice, it rounds (albeit neither is the current method - 14 digits aren't enough).)

Thank you for explaining it. I see. I remember I had changed it to make a certain case work properly when I was using it in the oauth2 module. Unfortunately I did not document why I changed it. I think I switched it back in the repository for now.

appgurueu · 2022-12-17T16:10:55Z

json.lua

    -- Encode
-    for i, v in ipairs(val) do
-      table.insert(res, encode(v, stack))
+    for i=1,length do


Fix your formatting

Yes fixed it for this function only. Tab structure in the original file is different than my tool. Didn't change the whole file.

appgurueu · 2022-12-17T16:10:59Z

json.lua

    end
    stack[val] = nil
    return "[" .. table.concat(res, ",") .. "]"

  else
    -- Treat as an object
    for k, v in pairs(val) do
+	--[[


Don't understand. Perhaps a more useful/productive comment?

Outcommented code is dirty; it's effectively leaving "trash" around in the codebase. If it isn't needed anymore, remove it; it'll still be in the Git history.

Ok, thanks. I usually keep some portions in my code to see some history immediately but you are right it is in the version history. I cleaned it up.

appgurueu · 2022-12-17T16:11:33Z

json.lua

+  -- Check whether to treat as a array or object
+  local array = true
+  local length = 0
+	local nLen = 0


Have you considered the worst-case this might create: What if I serialize {[1e9] = true}?

Missed all these. Revisiting after a long time. Thanks for catching that. Had not considered it at all. Been using it, did not have issues. Fixed it in the next commit by preventing sparse arrays greater than 100 (arbitrarility) length getting through. Seems like other json modules also handle sparse arrays in a undefined way. 100 should be good for many cases I suppose.

Yeah, it's a tricky issue, and behavior varies wildly. I believe ideally it should be left up to the user. Perhaps use something like a "density" factor as a threshold?

I have added a variable called SPARSELIMIT in the module that the user can change after requiring the module to change it. Default set it to 100.

ninjamonkey198206 · 2023-11-25T06:24:50Z

@appgurueu
Is there a reason for your abrupt and occasionally hostile behavior?

You're coming off as unnecessarily rude.

appgurueu · 2023-11-25T12:12:06Z

json.lua

+	local nLen = 0
+	local count = 0
+	for k,v in pairs(val) do
+		if (type(k) ~= "number" or k<=0) and not (k == "n" and type(v) == "number") then


What about fractional keys, like 4.2? I think you also need a check like k % 1 == 0 or math.floor(k) == k to check that k is (not) an integer.

Thank you, added k%1 ~= 0 in the condition.

appgurueu · 2023-11-25T12:29:10Z

@appgurueu Is there a reason for your abrupt and occasionally hostile behavior?

You're coming off as unnecessarily rude.

I'm not sure what you consider to be rude about my review. I made three comments:

Pointing out a possible worst-case scenario.
Commenting on the bad formatting.
Labeling a clear code smell (outcommented code) as "dirty". If it isn't dirty, is it clean? I believe(d) this to be self-evident; it does not need elaboration. Maybe I should have phrased this differently, as "oops, looks like you forgot some outcommented code here"?
Pointing out that using tostring on numbers is unreliable (if a certain format is expected). I should have elaborated on this; I didn't because it is something I am used to.

This was intended just as very "direct" feedback from a quick look.
In hindsight, some of it would have needed some elaboration.

If I came off as rude to @aryajur, that was not my intention, and I apologize for that.

aryajur · 2023-11-28T04:36:39Z

@appgurueu Is there a reason for your abrupt and occasionally hostile behavior?
You're coming off as unnecessarily rude.

I'm not sure what you consider to be rude about my review. I made three comments:

Pointing out a possible worst-case scenario.

Commenting on the bad formatting.

Labeling a clear code smell (outcommented code) as "dirty". If it isn't dirty, is it clean? I believe(d) this to be self-evident; it does not need elaboration. Maybe I should have phrased this differently, as "oops, looks like you forgot some outcommented code here"?

Pointing out that using tostring on numbers is unreliable (if a certain format is expected). I should have elaborated on this; I didn't because it is something I am used to.

This was intended just as very "direct" feedback from a quick look. In hindsight, some of it would have needed some elaboration.

If I came off as rude to @aryajur, that was not my intention, and I apologize for that.

I appreciate your feedback. Would always want to improve my work.

@appgurueu

…to control the length threshold of sparse arrays. Some bug fixes and improvements as suggested by @appgurueu

Improved table encoding code to handle sparse arrays. Floating point …

ad6ac0f

…numbers are encoding improved.

Updated code to allow array specifications using table.pack function.…

32a59c8

… The array detection code now allows for the array to have a key n with a number value to detect the array length.

nLen initialization fixed

f521e4a

appgurueu suggested changes Dec 17, 2022

View reviewed changes

Added check to prevent sparse arrays > 100 getting through

75127d2

appgurueu reviewed Nov 25, 2023

View reviewed changes

Improved indentation by using tab. Added module variable SPARSELIMIT …

0e6634a

…to control the length threshold of sparse arrays. Some bug fixes and improvements as suggested by @appgurueu

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved table encoding code to handle sparse arrays. #28

Improved table encoding code to handle sparse arrays. #28

aryajur commented Jul 19, 2020

Mysak0CZ commented Aug 18, 2020

aryajur commented Aug 19, 2020

appgurueu Dec 17, 2022

aryajur Nov 25, 2023

appgurueu Nov 25, 2023

aryajur Nov 28, 2023

appgurueu Dec 17, 2022

aryajur Nov 25, 2023

appgurueu Dec 17, 2022

aryajur Nov 25, 2023

appgurueu Nov 25, 2023

aryajur Nov 28, 2023

appgurueu Dec 17, 2022

aryajur Nov 25, 2023

appgurueu Nov 25, 2023

aryajur Nov 28, 2023

ninjamonkey198206 commented Nov 25, 2023

appgurueu Nov 25, 2023

aryajur Nov 28, 2023

appgurueu commented Nov 25, 2023 •

edited

Loading

aryajur commented Nov 28, 2023

Improved table encoding code to handle sparse arrays. #28

Are you sure you want to change the base?

Improved table encoding code to handle sparse arrays. #28

Conversation

aryajur commented Jul 19, 2020

Mysak0CZ commented Aug 18, 2020

aryajur commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninjamonkey198206 commented Nov 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appgurueu commented Nov 25, 2023 • edited Loading

aryajur commented Nov 28, 2023

appgurueu commented Nov 25, 2023 •

edited

Loading