Keep original encoding in declaration

Pull request type

Please check the type of change your PR introduces:

[ x] Bugfix
[ ] Feature
[ ] Code style update (formatting, renaming)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] Documentation content changes
[ ] Internationalization and localization
[ ] Other (please describe):

What is the current behavior?
https://github.com/veler/DevToys/issues/355
Issue Number: N/A
What is the new behavior?
After having the xml as a string, we replace the hard coded utf-16 to whatever the user input, or removing it if it was omitted

Other information

Quality check
Before creating this PR, have you:

[ ] Followed the code style guideline as described in CONTRIBUTING.md
[ ] Verified that the change work in Release build configuration
[ ] Checked all unit tests pass

Grats on your first PR to the repository @YSternlicht! 😊👍
One suggestion/comment. Here, you check for an XmlDeclaration element:
https://github.com/veler/DevToys/blob/7abeeaf6e9c6100df90cd8f9b9353acb26460e26/src/dev/impl/DevToys/Helpers/XmlHelper.cs#L102
I'd try moving this code block up a few lines to be above the stringBuilder declaration, and use pattern matching like this:
                if (xmlDocument.FirstChild is XmlNodeType.XmlDeclaration declaration)

This way, instead of using stringBuilder.Replace(), you can simply directly set the value of the declaration.Encoding property before the XmlDocument is written out, rather than manipulating the output after the fact.
Setting declaration.Encoding = string.Empty might remove the element from the declaration, though the documentation does say:

If an encoding attribute is not included, UTF-8 encoding is assumed when the document is written or saved out.

So that would need a bit of testing. It's not clear if that is literally just the output encoding, or if there would be a encoding="utf-8" element instead of no encoding element in the output.
Personally I think that setting XmlDeclaration.Encoding is probably more reliable than Replace(), since some future refactor might, e.g., change that default encoding somehow. You would end up accidentally replacing the first instance of "utf-16" somewhere else in the document. 😊
Also, here:
https://github.com/veler/DevToys/blob/7abeeaf6e9c6100df90cd8f9b9353acb26460e26/src/dev/impl/DevToys/Helpers/XmlHelper.cs#L104
You're scanning the xmlDocument output for the encoding string, rather than the input. Is that intended? Either way, if you move the code above the declaration of stringBuilder, you'll have to change it over to scanning the input:
                    Match match = Regex.Match(string.Split('>', 2)[0], @"(?<=encoding\s*=\s*"")[^""]*", RegexOptions.None);

The .Split('>', 2)[0] will make sure that you only match the first element in the input, and don't accidentally match any random encoding element somewhere else in the input. There's probably a more elegant way to do that, that's off the top of my head. 😊

Thanks @jwfxpr for your comment.
I agree with you that replacing the string after it has been placed in the string builder is not ideal.
However, a couple of issues forced me to do this.
There are 2 possible encodings to set, one on the xmlWriterSettings, and one on the xmlWriter . Settings property.
https://github.com/YSternlicht/DevToys/blob/7abeeaf6e9c6100df90cd8f9b9353acb26460e26/src/dev/impl/DevToys/Helpers/XmlHelper.cs#L68
https://github.com/YSternlicht/DevToys/blob/7abeeaf6e9c6100df90cd8f9b9353acb26460e26/src/dev/impl/DevToys/Helpers/XmlHelper.cs#L97

I agree with you that replacing the string after it has been placed in the string builder is not ideal. However, a couple of issues forced me to do this.
...

Seems like you've already investigated the situation. 😊 I didn't have the opportunity to play with the XmlDocument or XmlWriterSettings classes before writing the comment, and I think you've covered the same things I would have and I'd have come to the same conclusion as you. 👍

A possible solution could be to remove the StringBuilder and use a memory stream instead and return that. This will then default to utf-8. That could get very problematic as we will have to specify the encoding before we convert that stream into a string.

Yeah, I agree with you, changing the default to a different default with a more complex process isn't a great solution.
As far as I can see, if we're using System.Xml XmlDocument and XmlWriter to achieve the formatting, this is probably the best workaround available.
@veler, should a couple of simple test cases for this be added to src/tests/DevToys.Tests/Helpers/XmlHelperTests.cs in this PR?

Hi all,
Thank you very much @YSternlicht for opening this PR and proposing this change.

As far as I can see, if we're using System.Xml XmlDocument and XmlWriter to achieve the formatting, this is probably the best workaround available.

I agree with this. It seems to be the best workaround so far.

should a couple of simple test cases for this be added to src/tests/DevToys.Tests/Helpers/XmlHelperTests.cs in this PR?

Definitely! @YSternlicht , would you please be able to add some unit tests? :)

Hi @veler
Sure thing.
Been a busy week but will try get the tests written over the weekend.

Hi @veler
Sure thing.
Been a busy week but will try get the tests written over the weekend.

No worries, there's no hurry 😊
Thank you very much!

OK.
Tests written.


Installing

When installing it and running "go install" I got the following error message:
/srv/src/github.com/chrislusf/seaweedfs/weed# go install
../../../gocql/gocql/conn.go:9:2: cannot find package "context" in any of:
/usr/lib/go/src/pkg/context (from $GOROOT)
/srv/src/context (from $GOPATH)
Can anybody help please?

upgrade your Go version


Backend setup

Node, Express and MongoDB setup for the project
MVC based architecture setup
Do attach the screenshot of sample API

I would like to take on this task to set up the Node.js, Express, and MongoDB backend following the MVC architecture. I have experience working with these technologies and am confident that I can deliver a well-structured and scalable solution. I will follow the outlined steps, ensure proper code organization, and test the API endpoints.
Please assign me this task, and I'll get started right away!
Thanks,
Shubham Chamoli

@soham0005 could please assign this to me under the Hactoberfest label also

@shubham18102000 you can start working on this

@soham0005  thankyou

@shubham18102000 feel free to reach out for doubt, don't forget to star the repo <3

@shubham18102000 any updates?

@soham0005 working on register and signup page

@shubham18102000  Any updates?

@PRathod27 created login page and register page

@PRathod27  are you working on backend part '

@PRathod27  are you working on backend part '

No, I'm not working on it.

@soham0005

@soham0005   after login  page , we show the product items page  and having a cart option so person can buy that product

@shubham18102000 great work! just one enhancement, please do hash the password and then save it into database

@shubham18102000 if the initial setup of backend is done, do make the PR so that the rest backend work can be merged

@soham0005  pr is done


readbytes: index out of range 2

Describe the bug
i have a system with currently 5 volume servers. a few days ago i upgraded to the latest version and did also ec.encode. so many of my volumes are now ec encoded.
now i have a strange problem - when trying to access a file: "64,f9c55e5657cb73"
volume 64 is ec encoded and a lookup says:
{"volumeId":"64","locations":[{"url":"bm2:8280"},{"url":"bm6:8280"},{"url":"bm7:8280"},{"url":"bm8:8280"},{"url":"bm9:8280"}]}
so the file should be accessable from all 5 volume servers. but only from bm8 i getting the file - all other response with 404 not found.
looking in the logfile of one of those nodes says:
I1107 18:48:30 28041 store_ec.go:126] ReadEcShardNeedle vid 64 version:2: <nil>
I1107 20:04:23 28041 volume_server_handlers_read.go:92] read /64,f9c55e5657cb73 isNormalVolume false error: readbytes: index out of range 2

with most files the system works fine - but some requests are showing the behavier described
System Setup
Version: version 30GB 1.44 linux amd64
OS Version - mixed: Ubuntu 14.04.6 LTS and Ubuntu 18.04.3 LTS
weed volume -dataCenter=bm1 -rack=r1 -dir=/disks/weedfs1 -max 33 -ip=bm2 -mserver=bm2:9333,bm6:9333,bm7:9333 -port=8280 -port.public 8281
weed master -defaultReplication=001 -ip=bm2 -mdir=/data/weedfs -peers=bm2:9333,bm6:9333,bm7:9333

I suspect the volume servers can not talk to other volume servers via addresses like "bm6:8280".

No, this has been the first thing i have checked ... on all systems there is a /etc/hosts file where those hosts are defined.
But just to be sure i tried to change to -ip ipaddr:
Now lookup says:
{"volumeId":"64","locations":[{"url":"<IP_ADDRESS>:8280","publicUrl":"https://static.buchmedia.at/weed/bm2"},{"url":"<IP_ADDRESS>:8280","publicUrl":"https://static.buchmedia.at/weed/bm6"},{"url":"<IP_ADDRESS>:8280","publicUrl":"https://static.buchmedia.at/weed/bm7"},{"url":"<IP_ADDRESS>:8280","publicUrl":"https://static.buchmedia.at/weed/bm8"},{"url":"<IP_ADDRESS>:8280","publicUrl":"https://static.buchmedia.at/weed/bm9"}]}
(and yes all those servers can speak to each other)
but the same behavior - bm8 - aka <IP_ADDRESS> - is possible to deliver the file - every other volume server response with 404 not found.
same entries in logfile

can you set volume server logging level to 4. weed -v=4 volume ...  to enable detailed logging, and show the logs when processing one request, on volume servers other than bm8?

I1108 06:42:17  2676 volume_server_handlers_read.go:56] volume 64 reading f9c55e5657cb73 Size:0, DataSize:0, Name:, Mime:
I1108 06:42:17  2676 store_ec.go:138] read ec volume 64 offset<PHONE_NUMBER> size 2303926 intervals:[{BlockIndex:8 InnerBlockOffset:165773024 Size:2303952 IsLargeBlock:true LargeBlockRowsCount:2}]
I1108 06:42:17  2676 store_ec.go:291] read remote ec shard 64.8 from bm8:8280
I1108 06:42:17  2676 volume_server_handlers_read.go:90] read bytes 0 error readbytes: index out of range 2
I1108 06:42:17  2676 volume_server_handlers_read.go:92] read /64,f9c55e5657cb73 isNormalVolume false error: readbytes: index out of range 2

additional - log from bm8 - same request - works:
I1108 16:47:29 60627 volume_server_handlers_read.go:56] volume 64 reading f9c55e5657cb73 Size:0, DataSize:0, Name:, Mime:
I1108 16:47:29 60627 store_ec.go:138] read ec volume 64 offset<PHONE_NUMBER> size 2303926 intervals:[{BlockIndex:8 InnerBlockOffset:165773024 Size:2303952 IsLargeBlock:true LargeBlockRowsCount:2}]
I1108 16:47:29 60627 volume_server_handlers_read.go:90] read bytes 2303952 error <nil>

Could you please share all the volume 64 related files, 64.*, from bm8?

Hi,
yes no problem - in the meantime i did another test. i copied all datafiles from volume 64 to one server and started 5 local instances +master:
weed -logdir master master -mdir master -port 9500
weed -logdir bm2 volume -dir bm2 -mserver localhost:9500 -port 9502
weed -logdir bm6 volume -dir bm6 -mserver localhost:9500 -port 9506
weed -logdir bm7 volume -dir bm7 -mserver localhost:9500 -port 9507
weed -logdir bm8 volume -dir bm8 -mserver localhost:9500 -port 9508
weed -logdir bm9 volume -dir bm9 -mserver localhost:9500 -port 9509

same effect - download possible via port 9508 - but not via the other volume servers
so this should be no network issue ...
you can download the datafiles via:
https://static.buchmedia.at/weedtest/
files:
./bm8/64.ec08
./bm8/64.ec06
./bm8/64.ec07
./bm8/64.ecx
./bm8/64.ecj
./bm2/64.ec01
./bm2/64.ec00
./bm2/64.ec02
./bm2/64.ecx
./bm2/64.ecj
./bm7/64.ec11
./bm7/64.ecx
./bm7/64.ecj
./bm7/64.ec10
./bm7/64.ec09
./bm6/64.ec13
./bm6/64.ecx
./bm6/64.ecj
./bm6/64.ec12
./bm9/64.ec05
./bm9/64.ec04
./bm9/64.ec03
./bm9/64.ecx
./bm9/64.ecj

so for example: https://static.buchmedia.at/weedtest/bm2/64.ecx
if i understood correct - the ec format splits the volume in 10 chunks and adds 4 parity chunks and distibutes those 14 chunks among the volume servers.
so i think what happens is, when accessing a file which is in one of the local chunks the download works fine - but when redirecting to another volume server there is this error.
but i dont know why some files are working fine and others not ..

Thanks a lot for the details! I added a fix.

no problem - thanks for fixing - works !
a small detail i am not sure about - now with my compiled weed executable i am getting for a dir lookup:
http://bm2:9333/dir/lookup?volumeId=64
{"volumeId":"64","locations":[{"url":"bm2:8280","publicUrl":"https://static.buchmedia.at/weed/bm2"},{"url":"bm2:8280","publicUrl":"https://static.buchmedia.at/weed/bm2"},{"url":"bm2:8280","publicUrl":"https://static.buchmedia.at/weed/bm2"},{"url":"bm9:8280","publicUrl":"https://static.buchmedia.at/weed/bm9"},{"url":"bm9:8280","publicUrl":"https://static.buchmedia.at/weed/bm9"},{"url":"bm9:8280","publicUrl":"https://static.buchmedia.at/weed/bm9"},{"url":"bm8:8280","publicUrl":"https://static.buchmedia.at/weed/bm8"},{"url":"bm8:8280","publicUrl":"https://static.buchmedia.at/weed/bm8"},{"url":"bm8:8280","publicUrl":"https://static.buchmedia.at/weed/bm8"},{"url":"bm7:8280","publicUrl":"https://static.buchmedia.at/weed/bm7"},{"url":"bm7:8280","publicUrl":"https://static.buchmedia.at/weed/bm7"},{"url":"bm7:8280","publicUrl":"https://static.buchmedia.at/weed/bm7"},{"url":"bm6:8280","publicUrl":"https://static.buchmedia.at/weed/bm6"},{"url":"bm6:8280","publicUrl":"https://static.buchmedia.at/weed/bm6"}]}
so many duplicated entries - not a real problem - but why ?


Manage topics in a Kafka authenticated broker

This PR extends the kafka topics tasks of the amq_streams_broker role when the Kafka broker is authenticated.
The PR also extends the documentation describing how to create, describe or delete topics using the role.

@rmarting the amq_streams_broker_admin_mechanism is not defined, that's why this is failing. I've added the documented value to the default value:
amq_streams_broker_admin_mechanism: PLAIN
However, it stills fails (locally):
[2023-07-25 07:39:30,697] WARN These configurations '[//]' were supplied but are not used yet. (org.apache.kafka.clients.admin.AdminClientConfig)
[2023-07-25 07:39:30,895] ERROR [AdminClient clientId=adminclient-1] Connection to node -1 (localhost/<IP_ADDRESS>:9092) failed authentication due to: Unexpected handshake request with client mechanism PLAIN

@rpelisse Fixed, and CI is :green_heart:
Can you review it? Thanks


Let me set a custom color for rx.progress

https://reflex.dev/docs/library/datadisplay/progress/#api-reference
It's strange that the color of the loading bar can only be set via color_scheme, and there I can only choose the default radix themes.
I wanna use simply my own color, e.g. #FFF as the loading bar.

We used to have docs for this, but they appear to have been removed... https://github.com/reflex-dev/reflex-web/blob/masenf/changelog-0.4.9-and-friends/docs/library/datadisplay/progress-ll.md
Essentially you can set background to any valid css value and it should work

Can I try this one ?

@wassafshahzad just assigned you!

@picklelo  This can be closed. I forgot to add the linked PR

Merged in #3926


Is there a way to change datatype in onBeforeRender()?

I was trying to do this in onBeforeRender: function (data, node) { .. by setting the .type s I could find:
data.schema.type = "string";
data.elt.type = "string";
data.value = formatBytes(data.schema.default, 2); // Becomes "1.62 GB", for example

(yes, I am carrying database values in .default ... best place I could come up with).
This stumbling in the dark didn't work as I hoped, and I get JQuery complaints:
jquery.min.js:2 The specified value "1.62 GB" is not a valid number. The value must match to the following regular expression: -?(\d+|\d+\.\d+|\.\d+)([eE][-+]?\d+)?
In this wonky case, the database handles sizes in bytes, but in reality, these are indicative and user input most of the time. For that reason, it would make sense to let the user deal with them in more manageable presentation.
Is there a way to accomplish this? Render a value from interger to string, and when form is about to be submitted, can I write a code somewhere to return the field back into an integer type?

Hi
What about having a separate field (that is not submitted) in whatever format you like, and catch the submit event and copy the value, "de-format" it, and paste it in the real field, that is submitted ?
Have you explored this way of handling things ?

First version of this reply, while typing it, helped me to come up with answers to some questions about how to do that in my case... Reply version 2:
That sound like a reasonable thing to do. I just need to create display fields after retrieving data (value for JSONForm) and schema from API and then interpret values back to the actual fields before PUT'ing the entity back to API. I will try that!
BTW, I am extra happy that schema does not require those presentation related values. That makes so much more sense that "the next leading brand" -way of doing this, and the schema provided by the API (automatically parsed from the database table and its constraints) matches beautifully. ...leaving form with purely presentation related items. Nice and clear, making things easier!
Cheers!


Can't display video stream from django backend using iframe

Thank you so much for creating this plugin.
Have anyone tried using camera stream from localhost, which streaming from django local app, using OpenCV?
I have tried flask with no issue, however django app just refuse to connect.

I am sorry but I don't think this is related to this plugin since it only uses plain javascript without any third party libraries. I am not familiar with django. Do you see any error messages in the Developer Console of your browser?


Experimental fit tutorial

Thank you for your feedback. Most of it should be addressed by now. I don't know if I'm down adding other parameters fit because that is not how we did it, and experimentally we like fit parameters one by one instead of all at once. I need to think about it

@abocquet I made several corrections in 6376fabb6e9884984194c8f675e1088e0fa528ff. I'll let you look at the result, and check that you agree with the changes I made. Feel free to revert any change you don't agree with.
A few things left to do before merging:

[ ] Add experimental data on the github, and add the link to the tutorial
[ ] Update all figures and final g2 value with the changes I made (I didn't have access to the experimental data)
[ ] Fix the % skip: start and % skip: end (can be done in another PR, but ideally before merging this).
[ ] Make a final check that the provided code runs smoothly, without bugs.

Thanks for your work on this, I think it's going to be a great tutorial !


Investigate Github Actions

Investigate Github Actions to run CI and as a replacement for TravisCI

Needs to be migrated ASAP. Travis CI is becoming a blocker in updating to android-30. Apparently it does not accept the licenses.


rpc: allow users to set a persistent preference w.r.t the co-op fee rate

As is, we always use the same target to attempt to negotiate co-op close fees. Negotiation itself is difficult to really allow a users to express their true preference, but that's outside the scope of this issue. To make things a bit more predictable for users, they should be able to override the default fee rate that lnd uses when attempting a co-op close.

the default fee rate that lnd uses when attempting a co-op close

looking into this one, where can i find the default fee rate in the codebase?

@murtyjones the default rate is here: https://github.com/lightningnetwork/lnd/blob/f6f3ab5b0bd086ba3c4965bbdda0b6276893404b/peer/brontide.go#L2337. Right now it hard codes a 6 block conf estimate, which can be pretty high at times since it's what many default to.

@murtyjones, are you still looking into this one? If not, i'm keen to have a go.
Quick question: Would the best thing to do be to:
a) add this to the main lnd Config struct (something like --CoopCloseTargetConfs with default = 6) and then pass that config down to each peer Brontide Config and use it from there. (in this option, all channels will get the same value) .Or
b) Only have the config in the Brontide Config and then have an rpc method to change the value on a per-channel basis?

@murtyjones, are you still looking into this one? If not, i'm keen to have a go.
Quick question: Would the best thing to do be to:
a) add this to the main lnd Config struct (something like --CoopCloseTargetConfs with default = 6) and then pass that config down to each peer Brontide Config and use it from there. (in this option, all channels will get the same value) .Or
b) Only have the config in the Brontide Config and then have an rpc method to change the value on a per-channel basis?

Didn't end up getting to it, feel free!


OS X support

Per our conversation in #1, I figured I'd take a stab at removing the dependency on GNU coreutils, and it turned out to be fairly clean.
Caveats:

cp -u has been replaced with a bare cp and is thus theoretically slower; we waste cycles copying all files every time. But there's no measurable impact on my build times.

I realise this has now already been merged, but I would have objected to this on the following grounds:

realpath was not expected to be pre-installed on systems. E.g. my work machine which I regularly use for seL4 development did not come with realpath. Users were expected to install this, which I believe is widely available on POSIX systems.
cp -u was not intended to save time during the copy. I can't speak for this specific invocation, but generally cp -u has been used in the build system to prevent spurious time stamp updates causing rules that depend on the destination to be needlessly re-run.

In the case of copying the common-tool removing the -u flag does not cause a rebuild to happen. Elsewhere in the build system we use the -p flag, which preserves the timestamp. So while a bit of time is wasted redundantly copying, it does not snowball into resulting in rebuilding anything else.
As for realpath, I see no reason to depend upon an additional non standard utility when it's functionality is easy to emulate in a short shell expression.


Make target to run local changes

Description
Provide a make target useful for local development that builds all serverless images from local codebase (incl. runtimes, serverless workloads and operator) and runs them on local k3d cluster.
Make sure the instructions in README are up to date
Reason
Developers cannot run serverless from local sources

Is make run a good name and place for a target that runs serverless from main?

Please add archotecture diagram which describe the component relation


feat: add the rest of the orders api

Searching for example responses on GitHub is really funny because you get 30+ pages of the exact same xml 🤣

Codecov Report

Merging #29 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   99.61%   99.71%   +0.10%
==========================================
  Files          12       12
  Lines         259      356      +97
  Branches       27       32       +5
==========================================
+ Hits          258      355      +97
  Partials        1        1

Impacted Files
Coverage Δ

src/error.ts
100.00% <100.00%> (ø)

src/http.ts
98.41% <100.00%> (+0.63%)
:arrow_up:

src/sections/orders.ts
100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0711fe...892d0f4. Read the comment docs.


Multiple targets with different classes?

I understand that multiple targets can be assigned, but can multiple targets have their own set of classes be toggled as well?  Right now it seems the multiple targets must use the same classes.

Hi @jlahijani,
Thanks for taking time to submit this comment.
That's exactly that, yes. Multiple targets must use the same set of classes to toggle.
Changing that would complexify the stuff way too much, don't you think?

Thanks for the reply @Twikito.
The use case I had was the following.  Right now with TailwindUI, many of the sections require JavaScript.  TailwindUI has 3 options for these sections that require JavaScript:

use HeadlessUI (react)
use HeadlessUI (vue)
use HeadlessUI (alpine) -- this option doesn't exist yet but will in the near future (this is what I'm waiting for)
use your own Javascript

Easy Toggle State is actually a nice fit for many of the sections and could be the "use your own JavaScript" approach.
While experimenting, I came across a section that utilized an accordion.  It's the first one on this page (but you won't be able to see the interactive example unless you have a paid account):
https://tailwindui.com/components/marketing/sections/faq-sections
Anyway, when an accordion item is clicked, it must open the corresponding panel AND also apply a different CSS class to the item arrow to indicate an active state.  So, one trigger targets two different elements each with their own classes.
Easy Toggle State's accordion example circumvents this with some clever SCSS but with Tailwind, it's all about toggling classes themselves.
One thought I had is something like this (notice the commas):
<button
  data-toggle-target="#selector-1, #selector-2"
  data-toggle-class-on-target="class-on-selector-1a class-on-selector-1b, class-on-selector-2a class-on-selector-2b"
>

data-toggle-target="#selector-1, #selector-2" is already valid and possible with Easy Toggle State.
data-toggle-class-on-target="class-on-selector-1a class-on-selector-1b, class-on-selector-2a class-on-selector-2b" as it is right now will add everything to #selector-1.  I was thinking that if having a comma, it would match it with #selector-2 and apply it to that element instead.
Does this sound reasonable?

Well, this is interesting actually.
The problem here is that the target defined by data-toggle-target must be a css selector, so in your example #selector-1, #selector-2, which is a correct selector, tells to target two elements, but not to seperate them from toggling the specified classes.
The only way I see could be to enhance the selection and the class list to a be able to specify a table, sush as
<button
    data-toggle-target="[
        '#selector-1',
        '#selector-2'
    ]"
    data-toggle-class-on-target="[
        'class-on-selector-1a class-on-selector-1b',
        'class-on-selector-2a class-on-selector-2b'
    ]"
>

Seems doable, right?
But what about the trigger itself if I use data-toggle-class instead?

That looks like a great approach.
So data-toggle-class would add its classes to both the trigger and the target given current functionality:
https://twikito.github.io/easy-toggle-state/#toggling-combinations
... meaning it's optional shortcut of doing:
data-toggle-class-on-trigger='myclass1'
data-toggle-class-on-target='myclass1'

Therefore I think it would be fine to keep data-toggle-class functionality as is... meaning it should apply to all targets specified in the proposed syntax.

Hey @jlahijani 👋
I thought a bit more on that request, and I think that to avoid any behaviour conflict, I'll create a new property, such as
<button data-toggle-class-config="{
    trigger: 'myclass1 myclass2 …',
    '#targetId': 'myclass1 myclass2 myclass3 …',
    '.targetClass': 'myclass1 myclass3 myclass4 …'
}">

What do you think?

Sounds good to me.


Sales are not applied in checkout and orders

Fix #731

Current coverage is 54.02% (diff: 100%)

Merging #732 into master will increase coverage by 0.03%

@@             master       #732   diff @@
==========================================
  Files           105        105
  Lines          5523       5525     +2
  Methods           0          0
  Messages          0          0
  Branches        664        664
==========================================
+ Hits           2982       2985     +3
+ Misses         2427       2423     -4
- Partials        114        117     +3

Powered by Codecov. Last update 6d78d22...15a8bef


Add sub-system graphs

In a similar way to the per crate graphs (stageless branch), it could be useful to create per area graphs. For example, right now I'm using mod_debugdump to get an idea of which are all the UI related systems and how they relate.
So, I can either check the general view (which can be improved with #13) which is noisy; or check per crate images which lack information.
The idea would be generate graphs which add together various crates, for example, for UI it could be: winit, gilrs, input, text & ui.

So cool. It's like dark magic. 😅
I'll first focus on the colors issue, after that I can do some UI styling on this one. Maybe add a couple of convenience features: include presets, save filters state in URL… Also, given that it renders to SVG more fancy stuff can be done in JS-land, like searching for a system. 🤔
Btw, I suppose you're aware, but the include filter only accepts one.


Enable the feature that generates complex convex hulls

PR Details

Add Generator property to ConvexHullColliderShapeDesc instead of some properties.
Update version of ColliderShapeAsset from <IP_ADDRESS> to <IP_ADDRESS>.
Description

Xenko can already generate complex convex hulls with VHACD but that is not used in Game Studio.
This exposes it to use in Game Studio.
Computing them will take a lot of time (easily a minute or more by the default).

Related Issue

https://github.com/xenko3d/xenko/issues/81#issuecomment-455246809
Motivation and Context

We would like to create collider shapes that fit for Model.
Types of changes

[ ] Docs change / refactoring / dependency upgrade
[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] My change requires a change to the documentation.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

Thanks for the PR.
A few suggestions, after checking little bit better how it works.
I had a quick misunderstanding, I thought the VHACD lib was used exclusively in this case, but it seems used even when not doing decomposition.
As a result, it is just a bunch of optional parameters. The point to make an interface was to be able to replace it, but this is not necessary.
Considering this, my proposal would be to:

Rename type SimpleConvexHullGenerator to DecompositionParameters
Rename property ConvexHullColliderShapeDesc.Generator to ConvexHullColliderShapeDesc.DecompositionParameters
Remove the interfaces (you can use DecompositionParameters type directly)
Add a public bool Enabled { get; set; } in type DecompositionParameters
This will make property grid behave nicely with a Enable check box for the whole decomposition.
Then we can use SimpleHull = convexHullDesc.DecompositionParameters.Enabled.

Hope that makes sense.
Please tell me what you think.

I totally agree.
There is a difference between the name and the function of Generator.
About interfaces, I thought that those were needed to be able to select items in Game Studio.
However, I overlooked Enabled property.
That will be enough to be able to select them in Game Studio.
I will try it.
Thanks!

Merged, thanks!


Consider making In and Out types in FramedIo require 'static

This change would give up a little flexibility (which it's not clear we can actually make use of) for the sake of simplification in code that works generically with FramedIo.

Moved to https://github.com/tokio-rs/tokio-io/issues/26


Using GPS with KITTI dataset

I am facing problems while setting LIO-SAM with GPS for the KITTI dataset. I am getting a lot of sudden jumps in the output .
The package works fairly well without GPS. I think it's an issue with improper configuration. I seek some help regarding the same.

There are still some unsolved problems in LIO-SAM when a loop is closed or a GPS measurement is received. Any inputs are welcome from the community.

@TixiaoShan, my use-case requires creating globally correct point cloud maps. I haven't read the code thoroughly. Can you please list down the issues with including GPS measurements and loop closure, so as to keep track of the required changes and improvements?

Initial misalignment causes the /lio_sam/mapping/path to drift away from /odometry/gps. Further, when GPS data should be added to the pose graph, there is sudden jump in the pose. Hence leading to imperfect global poses.
I am trying with the default params on the Kitti_raw_data "2011_10_03_drive_0042 (4.5 GB)" dataset while it doesn't have position covariance in the /gps/fix data. Am I doing something wrong here?

You should tune the function that adds GPS factor in mapOptimization.cpp carefully. KITTI dataset doesn't give GPS covariance, so you have to change the threshold to figure out the best settings. When a GPS factor is added, imuPreintegration will be reset and cause a loss of initial guess in updateInitialGuess(). If the vehicle is moving very fast, the scan-matching may fail. You can disable these lines to help this problem. But it's not perfect. I am still trying to figure out solving matching failure when a GPS factor is added.

I tried with the above mentioned changes. I noticed that at points where positionCovariance is greater than the threshold, most of the GPS values though being correct, are neglected due to being relatively old or new w.r.t the pointcloud timestamp. This leads to error accumulation.
Later when a new GPS factor is added, heavy ISAM optimization takes place on a large no of nodes. During this time the imuPreintegration gets reset.
I tried on the kitti dataset. Please correct me if I am wrong.

I tried with and without GPS elevation integrated on KITTI dataset and tested with the ground truth data from Odometry sequences(I've less no. of keyposes so the data is scaled down, but the trend is almost same for X and Y axes)

Without using elevation from GPS, the Z axis has following results:

With GPS elevation, the following result is obtained for elevation in the final trajectory as compared to groundtruth :

Dotted line is ground truth

I tried with the above mentioned changes. I noticed that at points where positionCovariance is greater than the threshold, most of the GPS values though being correct, are neglected due to being relatively old or new w.r.t the pointcloud timestamp. This leads to error accumulation.
Later when a new GPS factor is added, heavy ISAM optimization takes place on a large number of nodes. During this time the imuPreintegration gets reset.
I tried on the kitti dataset. Please correct me if I am wrong.
I tried with and without GPS elevation integrated on KITTI dataset and tested with the ground truth data from Odometry sequences(I've less no. of keyposes so the data is scaled down, but the trend is almost same for X and Y axes)

Without using elevation from GPS, the Z axis has following results:

With GPS elevation, the following result is obtained for elevation in the final trajectory as compared to groundtruth :

Dotted line is ground truth
Should we use elevation from GPS or not?

@Pallav1299 You mentioned that the number of keyframes was different from the KITTI's ground truth.
I actually have the same problem. Did you evaluate the results using tum format for the trajectories?

@polbolso I didn't try visualizing in TUM or EUROC formats using EVO. I used KITTI format data. With KITTI format you would have to maintain the same number of poses to evaluate relative and absolute pose errors. This issue is solved by using timestamps in TUM format if I am not wrong. I'd suggest converting KITTI format poses to TUM for evaluation
in EVO.

Thanks @Pallav1299, I managed to combine the produced results with KITTI ground truth through TUM format. However, it seems that there is a clear misalignment issue. Have you experienced something similar?

Thanks @Pallav1299, I managed to combine the produced results with KITTI ground truth through TUM format. However, it seems that there is a clear misalignment issue. Have you experienced something similar?

I experienced same issue. There is a difference between vehicle axis and yours. You need to change the x y z poses reference to KITTI vehicle setup.


Add --set flag into upgrade command

https://github.com/istio/istio/issues/21778

I thought #21797 did this?

I thought #21797 did this?

we forgot to add the flag into the command

In response to a cherrypick label: new pull request created: #21948


SSP models within data directory

It would be desirable to create a /data/ssp_models dir that contains all models.

It is necessary.


Publish API Umbrella integration package under apinf organization

We now have an organization, called apinf, on Meteor.com. Move the brylie:api-umbrella package to apinf:api-umbrella. Also, invite other team members to the apinf organizaiton.

@Alapan be sure to provide us with your Meteor.com username here.


Attempting to use a locale that is not installed.

contrib/systemd/host-metering.services
has an environment variable that is invalid
at least with RedHat 7.9
Not sure it is fatal, but certianly produces log errors.
JP

Hey JP,
Thanks for raising this issue! I took a look and while the C.UTF-8 environment variable is indeed unavailable, PERL defaults to C when it fails to set the locale to C.UTF-8. So, in this case the error is non-fatal and doesn't affect the build.
I haven't updated contrib/systemd/host-metering.services since the build is unaffected and we hope to extend support to RHEL 8 & 9 as well. But as an aside, I updated our EPEL 7 mockbuilds to immediately use C, so that process is cleaner now thanks to your observation!
Please let me know if there are any concerns or anything is unclear!


refactor: move from io/ioutil to io and os packages

Reason for Change:

The io/ioutil package has been deprecated in Go 1.16 (See https://golang.org/doc/go1.16#ioutil). Since this project has been upgraded to Go 1.17 in #1042, this PR replaces the existing io/ioutil functions with their new definitions in io and os packages.
Issue Fixed:

Requirements:

[x] uses conventional commit messages

[ ] includes documentation
[ ] adds unit tests

Notes:

/azp run

@rbtr The Azure pipelines seems to be rate limited. I just received an email saying that the requests are being blocked.

we've got some unrelated pipeline problems being fixed; will need you to rebase the PR when that is all fixed and we can move this forward

/azp run

hey @Juneezee we squash and merge, so unless there's conflicts you don't need to keep rebasing a minor change like this 🙂 it confuses the CI

/azp run

tyvm @Juneezee


Analyzer does not report error for failed is! assertion in const constructors

Consider this simple, invalid Dart file:
// @dart=2.12
class Optional<T> {
  final bool present;
  final T? _value;

  const Optional.ofNullable(T? value)
    : assert(null is! T, 'T must be non-nullable for ofNullable()'),
      _value = value,
      present = value != null;
}

void main() {
  const Optional<int?>.ofNullable(null);
}

Attempting to run this file on 2.14.0-115.0.dev correctly prints:
repro.dart:13:8: Error: Constant evaluation error:
        const Optional<int?>.ofNullable(null);
              ^
repro.dart:7:21: Context: This assertion failed with message: T must be non-nullable for ofNullable()
    : assert(null is! T, 'T must be non-nullable for ofNullable()'),
                    ^

The analyzer does not report an error though:
Analyzing repro.dart...                1.0s

   info • repro.dart:4:12 • The value of the field '_value' isn't used. Try removing the field, or using it. • unused_field

1 issue found.

Surprisingly, dartanalyzer does not recognize the option --enable-asserts any more, but constant evaluation involving assertions should be enabled in any case.
So, I believe this is a bug: When T has the value int? the expression null is T should evaluate to true, and hence null is! T should yield false, so the assertion fails.

dart analyzer also does not accept --enable-asserts.
Is this the root cause? In other words, is this not a bug when asserts are not enabled. I think the code should not throw an assertion error when asserts are not enabled.

I believe the approach we agreed on was that the analyzer would always perform constant evaluation as if asserts were enabled. The reasoning, as I recall, was that asserts are primarily intended to be used during development, and the analyzer is a development-time tool, so it makes sense for them to always be treated as if they were enabled.

FWIW It looks like this is specific to the type check in the assertion. If I add another assertion like this:
  const Optional.ofNullable(T? value)
    : assert(null is! T, 'T must be non-nullable for ofNullable()'),
      assert(false),

Then the analyzer will report an "Evaluation of this constant expression throws an exception" as it should.


SEO friendlyness

Using local-router
When hitting "/" it should not redirect to "/defaultLocale" as this is not good for SEO. I believe when hitting defaultLocale it should not add "/defaultLocale" as it counts as a redirect and makes it's most of the times non-indexable.

I believe this is a duplicate of #72


MQTT port setting not used

The port value is ignored in the code, will use standard ports instead of user defined.

added to dev


Added Electron React Boilerplate, Awesome GraphQL, Koa GraphQL

Those projects are my personal attempts and have some potential to be awesome. : )

cool projects, thanks.


Cannot resolve symbol 'io' - import io.github.elyx0.reactnativedocumentpicker.DocumentPickerPackage;

I tried to import the package but there is an error
import io.github.elyx0.reactnativedocumentpicker.DocumentPickerPackage;

I want to use the v3

Any solution?


Can't use with github.com

Describe the bug
While trying to use with github I can't finalize the key configuration step:

When using the big green btn:

Translation (my browser is configured for FR language):

"Try with another security key"
"This security key isn't registered with this website"
[Close] [Retry]

I guess the "accept"/"deny" notification is expected to appear during the confirmation step but doesn't? 🤷🏼♂
Logs
ℹ  I try multiple times.
softu2f.log

I meet the same problem. I tested softu2f on this site, it works well, but not for GitHub.
It seems that GitHub does not trust Unknown device

I meet the same problem. I tested softu2f on this site, it works well, but not for GitHub.
It seems that GitHub does not trust Unknown device


We should considering about Apollo/GraphQL

Actually I was thinking about to convert my project which includes apollo graphQL, i18next, redux, universal rendering , mongoDB, restAPI etc. They were running on two separated project. One for graphQL and one for the app (on classic nodeJS and express). Then I found your repo and thought that "this is it".
In my opinion Apollo + graphQL has huge benefits and should be in this boilerplate too.

For the moment, I don't have any customer that requires GraphQL or Falcor. I think the same goes for @iRayzer. At the beginning, I was thinking of using Grapher, which surely ressembles a lot to Falcor, to lighten the burden of injecting specific data for specific routes. Still, it wasn't that mandatory for a first release.
There should be something to do about it in the next major release. The gotcha will be to ensure that server side caching will not increase the memory cost too much. GraphQL, Falcor or Grapher could help achieve it.

By the way, if you want to hop in the bandwagon and try to start the v2, we could add you to the organization and you could start digging on the V2. What do you think?

I need all of them above listed and maybe more. (animations,  localstorage  etc.) Maybe we can go another branches (or not) for custom needs if you want to keep the master one simple.
That would be nice if I could helpful.

Nice, nice, nice. I will setup the repo for adding a branching model. Certainly git-flow to ease our merges.

Btw this will be my first contribition so I need you lead me sometimes how to do things ;)

Done, I've added you as an owner of the organization. Welcome!
So from now on, we will be using git-flow.
Just create you own grahpql feature branch from the develop one and you're ready to go.
We can start a chat with Google Hangouts. My account is equivalent to my email (see my Github profile page). It will be easier than filling up this issue with posts.

Ok thanks. I accepted. I'm outside now. I will investigate the git-flow and create a hangouts account asap. Probably tomorrow. See you..

In the roadmap. Just closing this to keep our repo clean 😉


Parquet deserialization speeds slower on Linux

Describe the bug, including details regarding any error messages, version, and platform.
I'm debugging slow performance in Dask DataFrame and have tracked things down, I think, to slow parquet deserialization in PyArrow.  Based on what I know of Arrow I expect to get GB/s and I'm getting more in the range of 100-200 MB/s.  What's more is that this seems to depend strongly on the environment (Linux / OSX) I'm using.  I could use help tracking this down.
Experiment
I've isolated the performance difference down to the following simple experiment (notebook here):
# Create dataset
import pyarrow as pa
import pyarrow.parquet as pq
import numpy as np
import pandas as pd
import time
import io

x = np.random.randint(0, 100000, size=(1000000, 100))
df = pd.DataFrame(x)
t = pa.Table.from_pandas(df)

# Write to local parquet file

pq.write_table(t, "foo.parquet")

# Time Disk speeds

start = time.time()
with open("foo.parquet", mode="rb") as f:
    bytes = f.read()
    nbytes = len(bytes)
stop = time.time()
print("Disk Bandwidth:", int(nbytes / (stop - start) / 2**20), "MiB/s")

# Time Arrow Parquet Speeds

start = time.time()
_ = pq.read_table("foo.parquet")
stop = time.time()
print("PyArrow Read Bandwidth:", int(nbytes / (stop - start) / 2**20), "MiB/s")

# Time In-Memory Read Speeds

start = time.time()
pq.read_table(io.BytesIO(bytes))
stop = time.time()

print("PyArrow In-Memory Bandwidth:", int(nbytes / (stop - start) / 2**20), "MiB/s")

Results
I've tried this on a variety of cloud machines (intel/arm, VMs/metal, 8-core/64-core, AWS/GCP) and they all get fast disk speeds (probably cached), but only about 150MB/s parquet deserialization speeds.  I've tried this on two laptops, one a MBP and one a ThinkPad running Ubuntu and I get ...

MacBookPro: 1GiB/s PyArrow deserialization performance (what I expect)
Ubuntu/Thinkpad: 150MB/s PyArrow deserialization

In all cases I've installed latest release, PyArrow 13 from conda-forge
Summary
I'm confused by this.  I've seen Arrow go way faster than this.  I've tried to isolate the problem as much as possible to identify something in my environment that is the cause, but I can't.  Everything seems to point to the conclusion that "PyArrow Parquet is just slow on Linux" which doesn't make any sense to me.
I'd welcome any help.  Thank you all for your work historically.
Component(s)
Parquet, Python

Here is a rendered notebook from my Ubuntu Thinkpad to make the numbers more concrete: https://gist.github.com/mrocklin/526120bb5231cc5d9d4e3ca87fc09c68
This run was actually a bit faster than I usually see.  On Cloud VMs (even very nice ones) it's hard to get above 150MB/s on real data.

For reference, I ran your snippet above and repeated the timing part multiple times on a Linux (Ubuntu 20.04) Dell XPS 13 9380 (more than 4 years old, 8th gen Intel Core i7, 4 cores / 8 threads), and I get almost 2 GB/s for disk speed and around 1 GB/s for reading (just under for from file, just above for in-memory).
(so at least it's not a simple mac vs linux issue)
One environment characteristic that will significantly influence those numbers is the parallelization (the Parquet reading will by default use all the available cores). So it might be worth to run those timings with and without threads enabled, to ensure it's not related to bad scaling on that front.
On my laptop, I get the expected 3-4x speedup with threads enabled (the numbers above), as I get around 250-300 MB/s using use_threads=False.

I ran a couple of pyspy benchmarks on pure pq.read_table downloading from S3. I ran two tests, one with column projection and one with bulk reading. Both show basically the same profile but with different weighting of components.
This profile shows the case where I'm reading a file and are selecting about half it's columns (a mix between different dtypes)

Note how the read_table request is split into three parts

A HEAD request that infers whether the provided path is a file or a directory https://github.com/apache/arrow/blob/ac2d207611ce25c91fb9fc90d5eaff2933609660/python/pyarrow/parquet/core.py#L2481 This is latency bound which on S3 is typically 100-200ms but can vary quite strongly. This is a request we cannot cache or use to pre-fetch any kind of data. In this example, this alone took 20% of the entire read time
The initialization of the FileSystemDataset object. In native profiles, this points to arrow::dataset::Fragment::ReadPhysicalSchema so I assume this is fetching the footer. This is probably unavoidable but at least this request could be used to pre-fetch some payload data but I'm not sure if this is actually done (I guess not since, the buffer_size kwarg is zero by default). In this specific example, this is about 10% of the read
The final section is now the actual reading of the file.

So, that's 30% where we're doing nothing/not a lot? I'm not sure at which point the pre_buffering can kick on or how this works. This stuff does not show up in my profile since it's the arrow native threadpool.
At least this initial HEAD request appears to be bad, particularly if we're fetching just a couple of columns from otherwise already small-ish files. The file I was looking at is one of the TPCH lineitem files which in our dataset version is 22.4MiB large.

About (1) some optimization will be included later, see:

https://github.com/apache/arrow/pull/37868 (This patch should be revisited )

Seems we can enable larger prefetch-depth to arrow fetching multiple files concurrently.

Sorry, I just realize that my comment is also slightly off topic. The OP discusses pure deserialization without S3 in between

FWIW I slightly modified the above script to run each operation N times since I noticed quite some variance on my machine (M1 2020 MacBook)

# Create dataset
import pyarrow as pa
import pyarrow.parquet as pq
import numpy as np
import pandas as pd
import time
import io

x = np.random.randint(0, 100000, size=(1000000, 100))
df = pd.DataFrame(x)
t = pa.Table.from_pandas(df)
niterations = 20

# Write to local parquet file

pq.write_table(t, "foo.parquet")

# Time Disk speeds

start = time.perf_counter()
for _ in range(niterations):
    with open("foo.parquet", mode="rb") as f:
        bytes = f.read()
        nbytes = len(bytes)
stop = time.perf_counter()
print("Disk Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

# Time Arrow Parquet Speeds

start = time.perf_counter()
for _ in range(niterations):
    _ = pq.read_table("foo.parquet")
stop = time.perf_counter()
print("PyArrow Read Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

# Time In-Memory Read Speeds

start = time.perf_counter()
for _ in range(niterations):
    pq.read_table(io.BytesIO(bytes))
stop = time.perf_counter()

print("PyArrow In-Memory Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

# Time In-Memory Read Speeds

start = time.perf_counter()
for _ in range(niterations):
    pq.read_table(io.BytesIO(bytes)).to_pandas()
stop = time.perf_counter()

print("PyArrow (to_pandas) Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

and I get
Disk Bandwidth: 5154 MiB/s
PyArrow Read Bandwidth: 2294 MiB/s
PyArrow In-Memory Bandwidth: 2439 MiB/s
PyArrow (to_pandas) Bandwidth: 1142 MiB/s

@jorisvandenbossche have you used the same conda forge build for your measurements or did you build it yourself? It would be nice to rule out any build differences

Ok, fun experiment. I wrapped the above script in a function run_benchmark and ran this on my machine...

Looks like the simple fact that we're running this in the dask environment is slowing us down quite a bit. This also biases most/all Coiled-based cloud benchmarks

Code
# Create dataset
import pyarrow as pa
import pyarrow.parquet as pq
import numpy as np
import pandas as pd
import time
import io
def run_benchmark():
    from distributed.worker import print
    x = np.random.randint(0, 100000, size=(1000000, 100))
    df = pd.DataFrame(x)
    t = pa.Table.from_pandas(df)
    niterations = 20

    # Write to local parquet file

    pq.write_table(t, "foo.parquet")

    # Time Disk speeds

    start = time.perf_counter()
    for _ in range(niterations):
        with open("foo.parquet", mode="rb") as f:
            bytes = f.read()
            nbytes = len(bytes)
    stop = time.perf_counter()
    print("Disk Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")
    # Time Arrow Parquet Speeds

    start = time.perf_counter()
    for _ in range(niterations):
        pq.read_table("foo.parquet")
    stop = time.perf_counter()
    print("PyArrow Read Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

    # Time In-Memory Read Speeds

    start = time.perf_counter()
    for _ in range(niterations):
        pq.read_table(io.BytesIO(bytes))
    stop = time.perf_counter()

    print("PyArrow In-Memory Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

    # Time In-Memory Read Speeds

    start = time.perf_counter()
    for _ in range(niterations):
        pq.read_table(io.BytesIO(bytes)).to_pandas()
    stop = time.perf_counter()

    print("PyArrow (to_pandas) Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

run_benchmark()

from distributed import Client
client = Client()

client.submit(run_benchmark).result()

Is pyarrow using either one of OMP_NUM_THREADS, MKL_NUM_THREADS, OPENBLAS_NUM_THREADS to infer how large the threadpool is allowed to be?

To be clear, my experiment in this issue was run without Dask and without Coiled.  I wanted to isolate things.

I'm curious, for deserializing integers is this expected performance?  I would have thought that for this kind of data (just ints) we'd be closer to GB/s speeds.  I'm curious, what is the slow part here?  Is there some intense compression or something?

have you used the same conda forge build for your measurements or did you build it yourself? It would be nice to rule out any build differences

I'm also quite curious about this.

I was using a conda-forge install for the above numbers, no custom build.

But for linux you mentioned above a similar number with threads. So that means you see hardly any perf improvement with threads on linux?

Is pyarrow using either one of OMP_NUM_THREADS, MKL_NUM_THREADS, OPENBLAS_NUM_THREADS to infer how large the threadpool is allowed to be?

Yes, it seems we are using OMP_NUM_THREADS (and otherwise check std::thread::hardware_concurrency(), which I think also doesn't always give the correct number, eg in a container).
You can also manually override this with pa.set_cpu_count().

Yes, it seems we are using OMP_NUM_THREADS (and otherwise check std::thread::hardware_concurrency(), which I think also doesn't always give the correct number, eg in a container), see the relevant code.
You can also manually override this with pa.set_cpu_count().

Yes, thanks. I already found that pyarrow is setting the CPU threadpool to one inside of dask regardless of the env settings. I already tested a little with set_cpu_count but so far we haven't seen the hoped-for speedup

To find a standardized shared compute environment I tried this on Google Colab.
I found that x.copy() ran in 2 GB/s and pq.read_table(io.BytesIO(bytes)) ran in 180 MB/s.

I found that x.copy() ran in 2 GB/s and pq.read_table(io.BytesIO(bytes)) ran in 180 MB/s.

I'm not sure if this comparison is actually fair and valid. Parquet -> Arrow has to do a nontrivial amount of work. Even your random data is encoded and compressed. (See pq.ParquetFile("foo.parquet").metadata.to_dict() to inspect the metadata)

I also ran this on colab and got something like this
Disk Bandwidth: 1636 MiB/s
PyArrow Read Bandwidth: 231 MiB/s
PyArrow In-Memory Bandwidth: 220 MiB/s

from your benchmark output. I went along and ran
import pickle
pickled_df = pickle.dumps(x)
compressedb = pa.compress(pickled_df, "SNAPPY")
nbytes = len(compressedb)
start = time.time()
pa.decompress(compressedb, decompressed_size=len(pickled_df), codec="SNAPPY")
stop = time.time()print("SNAPPY Decompress Bandwidth:", int(nbytes / (stop - start) / 2**20), "MiB/s")

which gives me
SNAPPY Decompress Bandwidth: 199 MiB/s
so we're moving in the same vicinity as the parquet read.

Cool.  What I'm reading from you are a couple of insights:

Arrow uses SNAPPY compression by default
SNAPPY is performing around 200 MB/s on these machines

I'll add my understanding, which is that I also expect SNAPPY to operate at GB/s speeds, but for some reason it's not here.
On Colab I also get 200MB/s as you do, but on my MacBook I get 767 MB/s
This probably allows us to dive a bit deeper into the problem.

But for linux you mentioned above a similar number with threads. So that means you see hardly any perf improvement with threads on linux

Yes. That's correct. To be clear though, I'm currently more confused about only getting 150-200 MB/s deserializing integers on a single thread. That seems very strange to me.

Yes, I understand (and dask uses use_threads=False anyway, so mostly depends on this single threaded performance). But then to not mix too many different issues at once, it might be better to focus the various timings in this issue on single threaded performance.

Parquet -> Arrow has to do a nontrivial amount of work

Parquet is indeed a complex file format. In addition to the decompression, there is also the decoding (although the file here will use dictionary encoding, and that should be quite fast I would expect. Also quickly testing plain and delta_binary_packed encodings, and that actually gives slower reads than the default in this case).
I was also wondering if we could have an idea which bandwidth one can expect for just the decompression, to have some point of comparison. The snappy readme (https://github.com/google/snappy) itself mentions decompression at 500MB/s for Intel Core i7. Running the snippet of Florian above, I actually only get around 100MB/s for the SNAPPY decompression..

Arrow uses SNAPPY compression by default

Quickly testing with another compression (pq.write_table(t, "foo_lz4.parquet", compression="lz4"), I get consistently faster reads with LZ4 compared to SNAPPY for this dataset, but only around 5-10% faster. Not a huge difference, but so in general one can always tweak the encoding and compression settings for their specific datasets to achieve optimal read performance.
Using no compression at all (compression="none") also gives some speed-up (but of course trading storage size with read speed, and on eg S3 that might not even be beneficial)

Re-running the benchmarks with a slightly adapted script from above (single threaded, different compressions), and ensuring I run it while having no other applications running, I actually get quite decent single threaded performance:

Code

import pyarrow as pa
import pyarrow.parquet as pq
import numpy as np
import pandas as pd
import time
import io

# Create datasets
x = np.random.randint(0, 100000, size=(1000000, 100))
df = pd.DataFrame(x)
t = pa.Table.from_pandas(df)
pq.write_table(t, "foo.parquet")
pq.write_table(t, "foo-lz4.parquet", compression="lz4")
pq.write_table(t, "foo-uncompressed.parquet", compression="none")

def run_benchmark(fname):

    niterations = 20

    # Time Disk speeds

    start = time.perf_counter()
    for _ in range(niterations):
        with open(fname, mode="rb") as f:
            bytes = f.read()
            nbytes = len(bytes)
    stop = time.perf_counter()
    print("Disk Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")
    # Time Arrow Parquet Speeds

    start = time.perf_counter()
    for _ in range(niterations):
        pq.read_table(fname, use_threads=False)
    stop = time.perf_counter()
    print("PyArrow Read Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

    # Time In-Memory Read Speeds

    start = time.perf_counter()
    for _ in range(niterations):
        pq.read_table(io.BytesIO(bytes), use_threads=False)
    stop = time.perf_counter()

    print("PyArrow In-Memory Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

    # Time In-Memory Read Speeds

    start = time.perf_counter()
    for _ in range(niterations):
        pq.read_table(io.BytesIO(bytes), use_threads=False).to_pandas(use_threads=False)
    stop = time.perf_counter()

    print("PyArrow (to_pandas) Bandwidth:", int(nbytes / ((stop - start) / niterations) / 2**20), "MiB/s")

In [3]: run_benchmark("foo.parquet")
Disk Bandwidth: 2052 MiB/s
PyArrow Read Bandwidth: 436 MiB/s
PyArrow In-Memory Bandwidth: 459 MiB/s
PyArrow (to_pandas) Bandwidth: 280 MiB/s

In [4]: run_benchmark("foo-lz4.parquet")
Disk Bandwidth: 2100 MiB/s
PyArrow Read Bandwidth: 516 MiB/s
PyArrow In-Memory Bandwidth: 569 MiB/s
PyArrow (to_pandas) Bandwidth: 323 MiB/s

In [5]: run_benchmark("foo-uncompressed.parquet")
Disk Bandwidth: 2092 MiB/s
PyArrow Read Bandwidth: 667 MiB/s
PyArrow In-Memory Bandwidth: 730 MiB/s
PyArrow (to_pandas) Bandwidth: 409 MiB/s

And the file sizes are 258, 255 and 293 MB, respectively (so the actual speedup for uncompressed is a bit lower than what the above gives, because it's reading more MBs. But it's still faster in terms of seconds to read)

PyArrow In-Memory Bandwidth: 730 MiB/s

This seems like a rather low number for uncompressed integer data. What is the exact encoding used by the integer column? The metadata display above is a bit ambiguous as it lists three encodings :-)

Ok, so after investigating this, one factor is that pq.write_table by default tries to use dictionary encoding, and the heuristic to stop using dictionary encoding is when the dictionary page size reaches the dictionary_pagesize_limit parameter, which is set to a very high default of 1MB.
However, the benchmark numbers are also largely skewed by the fact that the MB/s figure is computed relatively to the compressed and encoded size, not on the final in-memory size.

With that in mind, here are updated results against the actual in-memory size:

with encoding RLE_DICTIONARY:

PyArrow Read Bandwidth for 'foo.parquet': 1274 MiB/s
PyArrow In-Memory Bandwidth for 'foo.parquet': 1355 MiB/s
PyArrow Read Bandwidth for 'foo-lz4.parquet': 1533 MiB/s
PyArrow In-Memory Bandwidth for 'foo-lz4.parquet': 1645 MiB/s
PyArrow Read Bandwidth for 'foo-uncompressed.parquet': 1639 MiB/s
PyArrow In-Memory Bandwidth for 'foo-uncompressed.parquet': 1775 MiB/s

with encoding PLAIN:

PyArrow Read Bandwidth for 'foo.parquet': 493 MiB/s
PyArrow In-Memory Bandwidth for 'foo.parquet': 515 MiB/s
PyArrow Read Bandwidth for 'foo-lz4.parquet': 1323 MiB/s
PyArrow In-Memory Bandwidth for 'foo-lz4.parquet': 1478 MiB/s
PyArrow Read Bandwidth for 'foo-uncompressed.parquet': 1761 MiB/s
PyArrow In-Memory Bandwidth for 'foo-uncompressed.parquet': 2428 MiB/s

with encoding DELTA_BINARY_PACKED:

PyArrow Read Bandwidth for 'foo.parquet': 1573 MiB/s
PyArrow In-Memory Bandwidth for 'foo.parquet': 1644 MiB/s
PyArrow Read Bandwidth for 'foo-lz4.parquet': 1606 MiB/s
PyArrow In-Memory Bandwidth for 'foo-lz4.parquet': 1657 MiB/s
PyArrow Read Bandwidth for 'foo-uncompressed.parquet': 1691 MiB/s
PyArrow In-Memory Bandwidth for 'foo-uncompressed.parquet': 1801 MiB/s

And now the file sizes:

with encoding RLE_DICTIONARY (the default):

-rw-rw-r-- 1 antoine antoine 255224210 oct.  24 16:53 foo-lz4.parquet
-rw-rw-r-- 1 antoine antoine 258035659 oct.  24 16:53 foo.parquet
-rw-rw-r-- 1 antoine antoine 292773361 oct.  24 16:53 foo-uncompressed.parquet

with encoding PLAIN:

-rw-rw-r-- 1 antoine antoine 414232019 oct.  24 16:54 foo-lz4.parquet
-rw-rw-r-- 1 antoine antoine 448751689 oct.  24 16:54 foo.parquet
-rw-rw-r-- 1 antoine antoine 800112507 oct.  24 16:54 foo-uncompressed.parquet

with encoding DELTA_BINARY_PACKED:

-rw-rw-r-- 1 antoine antoine 228584003 oct.  24 16:55 foo-lz4.parquet
-rw-rw-r-- 1 antoine antoine 227823921 oct.  24 16:55 foo.parquet
-rw-rw-r-- 1 antoine antoine 227811357 oct.  24 16:55 foo-uncompressed.parquet

... meaning that DELTA_BINARY_PACKED is probably a very good choice for integer columns.

(here's the benchmark code used for the numbers above: https://gist.github.com/pitrou/1713b6d32111dac9102e87be1b5ac887 )

Also shows, btw, that MiB/s can be a very misleading metric. In the end, you're interested in reading values, not bytes, so I find it more useful to reason in terms of Mitems/s.

Summarizing the numbers I get with the edits from Antoine, for the defaults (snappy compression, dictionary encoding): around 1350 MiB/s for reading from disk, around 1450 MiB/s for reading from memory (single threaded performance, using my Linux laptop: Dell XPS 13 9380 with Ubuntu 20.04, 8th gen Intel Core i7)

Just a heads up. My current working theory is that the parquet deserialization performance is roughly where it is supposed to be (but honestly I don't know) but what we're seeing is rather an artifact of storage latency due to too many small reads. I'll keep that conversation separate from this issue here, see https://github.com/coiled/benchmarks/issues/1125#issuecomment-1778995537

Still, I think the question this issue is asking is whether the performance for the deserialization (w/out IO) we're seeing is what is expected and if the severe slowdown on other hardware is expected.

I should probably raise another issue with "how to use arrow to read parquet data from the cloud quickly" and ask for help on the general problem, and point to this issue as a sub-part of that.

Still, I think the question this issue is asking is whether the performance for the deserialization (w/out IO) we're seeing is what is expected and if the severe slowdown on other hardware is expected.

I think it would be nice if you could re-do your benchmarks with a proper MiB/s metric (i.e., computed on decoded size, not file size). Also, please ensure that the different machines read the same files.

My intuition says that Arrow deserialization should be much faster than S3 download speeds, and so we can stop when we're within ~80% of S3 download speeds

But this is Parquet deserialization, not Arrow deserialization :-) Parquet features many encodings and compression schemes, most of which trade CPU overhead for disk footprint (and network transmission times).
You're also looking at single-thread Parquet decoding speed here, but presumably you're going to download and decode multiple files simultaneously, which will give you a multiple of the decoding speed and may reach S3 download limits.
(also, I don't know what the expected download speeds from S3 are...)

See also https://gist.github.com/mrocklin/9eca141688d03bbb6c375e53bee35c6d

Reading from S3 might better enable PreBuffer, and making some Cache input, it would do some:

Merging adjacent S3 IO request with some condition.
PreBuffer some column-chunk

Generally, I think S3 has higher latency, but might has high bindwidth. So if memory is enough, PreBuffer would help

See also https://gist.github.com/mrocklin/9eca141688d03bbb6c375e53bee35c6d

For the record, what is the in-memory data size in this example? (table.nbytes)

Also. The benchmark is great
From the paper ( https://www.durner.dev/app/media/papers/anyblob-vldb23.pdf ) perspective, the io-size also important.

I think that those files were relatively small.  Unfortunately we've since replaced them with larger files.  I'll re-run everything and include table size numbers as well, hopefully within the next hour.

And some machine might have different bandwidth, this should also be taken into account

Certainly this is true.  This is why I record S3 bandwidth at the beginning.  That gives us a baseline.  My goal isn't to get a high MiB/s number.  It's to get a full pipeline that is close to S3 bandwidth.  I hope that S3, rather than pyarrow.parquet, is the bottleneck.

@mrocklin
In best case, the IO and CPU is pipelined, we will waiting for the first group of IO finished, and advacne does the handling logic
However, there're few points might affect this:

fetching in thread pool doesn't ensure priority. So some part might with the different order than expected. For example, there a 2 column chunks, each chunks has 5 IO. The 5th io might finish earier than the first one
As a result, the pattern might become: Waiting for IO -> do cpu things..

To optimizing this, the dataset api might split a file to different row-group, and has a row-group reader (aka ParquetFragment..). The fragment will be io and read parallelly. This might helps a bit.

re: the pipelining/IO discussion, you may find the discussion here interesting: https://lists.apache.org/thread/cdfkm8oflm2zvd25yn4k6gh2o7pc9z88
Some (but not all) of those proposals were implemented in Arrow ("pre-buffering" primarily), though pre-buffering is probably not the ideal way to implement it (too much memory usage). One thing that didn't make it was the global concurrency manager, which would have approximated priority by not actually issuing reads for a file until all reads for previous files have been issued (of course, this only makes sense if there's an ordering between the files in the first place - not necessarily true for dataset)
That said, I believe datasets does parallelize at the row-group level already @mapleFU

That said, I believe datasets does parallelize at the row-group level already

I think user uses ParquetFile api here. Which is apart from Dataset API. When a file contains multiple row-groups, it might be slower(Or might not).

Hmm, s3fs does some readahead. If you are scanning the whole file, this probably helps. However, Arrow's FS + pre-buffer should be better in general (readahead is actively harmful if you aren't scanning the whole file)

Yeah, in practice s3fs actually has some intelligent system buried within it for exactly this use case.  See https://developer.nvidia.com/blog/optimizing-access-to-parquet-data-with-fsspec/
(I actually just learned about this)

Hmm, s3fs does some readahead. If you are scanning the whole file, this probably helps. However, Arrow's FS + pre-buffer should be better in general (readahead is actively harmful if you aren't scanning the whole file)

In my opinion, when reading the same file, if user is using ArrowFS, before 14.0, the default options is not Lazy, so, the file will pre_buffer all row-groups and all columns at once. I don't know how could be a times faster... Maybe I need to re-produce this myself to take a look 🤔

Ah, that means fsspec actually implements the same optimization as PyArrow now.

See also https://gist.github.com/mrocklin/9eca141688d03bbb6c375e53bee35c6d

@lidavidm from user's test in this case s3fs might be faster 😭:-(

Things are still very much up in the air.  There's a lot of sensitivity to file size, machine type, etc..
Clearly there is room for improvement with Arrow Filesystem.  I think that Dask developers are generally hoping that, with some configuration changes, we can get Arrow filesystem to be fast enough to be our default.  For example, I suspect that sometimes s3fs wins because ...

We can change the chunksize (s3 seems to work best with 16 MB chunks)
We can super-saturate threads (s3 seems to work well with 2-3 threads per core) (I suspect that arrow might be limiting concurrency more than is ideal)

However, s3fs is slow for other reasons, like Python and the GIL (we can't scale it up on larger workers as easily).  Arrow seems to have more potential, but is currently harder for us to optimize.
If it makes you feel better, I can easily produce a similar notebook that shows Arrow doing way better 🙂
There's more conversation going on here: https://github.com/coiled/benchmarks/issues/1125

We can super-saturate threads (s3 seems to work well with 2-3 threads per core) (I suspect that arrow might be limiting concurrency more than is ideal)

https://github.com/apache/arrow/blob/73589ddd60e4cbcd860102871692541989ea38c6/cpp/src/arrow/io/interfaces.cc#L389-L415
Maybe we should default this to 2*numcpus or similar, instead of hardcoding 8 (!)

This may also differ strongly between local / low-latency systems, and cloud-object / high-latency systems.

I'm currently hoping that the following has an effect:
import pyarrow as pa
pa.set_io_thread_count(...)

I'm not seeing much response in benchmarks yet (but that could be due to other reasons)

Why would we use 2*numcpus for IO threads? There's an issue open somewhere for per-filesystem thread pools which I think would be a fruitful avenue.

For S3, we've found that 2-3x numcpus is pretty good.  One get about 50MB/s per S3 connection, and total aggregate S3 bandwidth on Amazon is correlated with machine size (larger machines with more cores have more bandwidth).  This scales linearly for modestly sized machines, such that 2-3x ends up being a good general rule.
This is made more explicit at the top of the notebook I shared (using more threads with raw S3 access results in greater aggregate bandwidth).

Issue for IO thread pool sizes: https://github.com/apache/arrow/issues/30552

This is made more explicit at the top of the notebook I shared

Can you give a link again? There are many notebooks and benchmark numbers in this issue already :-)

Here is a new one with a dataset with larger file sizes (128 MB on disk):
https://gist.github.com/mrocklin/c1fd89575b40c055a9be77b2a47894df
Arrow filesystem does well, but still only about half of what I suspect is possible.

Thanks @mrocklin . Perhaps this is asking too much, but what are the numbers if you first download the files locally and have Arrow work on the local filesystem?

Sure.  One moment.

I've updated the same notebook, see the bottom:
https://gist.github.com/mrocklin/c1fd89575b40c055a9be77b2a47894df

Thanks. Those numbers are concerning. I'll try to investigate local performance when I find some time.

Thanks Antoine.  I appreciate it.  I'm not expecting anything from you, but if you happen to come up with something before the middle of next week I'd be excited to use it in some benchmarks I'm currently working on.

OK, I think I've gotten the rest of the Dask toolchain to the point where I'm now CPU-bound by Arrow deserialization.  If there is anything I can do to to accelerate investigation/improvements here (for example, buying the right person a beer or a vacation on a tropical island) I'd be happy to do so.

I'm now CPU-bound by Arrow deserialization

What do you mean by "Arrow deserialization"? Is this parquet related (.i.e decompress, loading rep-def, decoding ) or parquet to arrow (building arrow data type) or casting arrow to other?

Sorry for being non-specific.   I meant PyArrow Parquet performance specifically.  My apolgoies for the lack of precision.
In particular, the configuration setting I'm using is
pq.ParquetFile(s3_filename, pre_buffer=True).read(
    columns=columns,
    use_threads=False,
    use_pandas_metadata=True,
)

As mentioned above on a 4 core machine when running this in parallel with 4-8 threads I can get up to about 200-250 MB/s.  CPU utilization during this period is high.
I'm also open to other APIs, but that has performed the best for me in my circumstances so far.

nice, glad to see that!

This issue might be related? https://github.com/apache/arrow/issues/38591

@eeroel that's an interesting issue.  Thank you for sharing and fixing.  The chart you share makes the problem super-clear.  I'll be curious to see how it impacts observed performance.
I'm still curious about the high CPU-utilization.  I was chatting separately with Wes and he mentioned the following (I don't think I'm sharing anythign private)

I would guess that collecting C++ profile data via perf/flamegraph would tell an interesting story about what's taking up the most time in the Parquet deserialization (you generally need to recompile things with -fno-omit-frame-pointer to get reasonable looking profiles). Snappy is a common culprit

Maybe I should try with non-snappy-compressed data and see how things behave.  That would probably help to bisect the problem space.
I'm going to summarize a few possible directions to investigate that came out of this discussion:

Try again after #38591
Try without snappy compression
Collect C++ profile data with perf/flamegraph
Look at changing download chunk sizes from 2MB to something larger like 5MB or 10 MB for object-store filesystems (I think I recall  someone saying 2MB was the default, but that that was probably decided for local posix filesystems)

No obligation of course for anyone to do this work.  This comment is as much for people who I work with as it is for possible Arrow maintainers if they're interested (I hope that you're interested!).  For convenience, the notebook I was using above.  cc @fjetter

Try again after https://github.com/apache/arrow/issues/38591

Note that I assume you were doing your initial benchmarks with pyarrow 13.0 (because 14.0 was not yet released when the thread started), and I think this is only a regression in 14.0

Try without snappy compression

Antoine posted some numbers above (eg https://github.com/apache/arrow/issues/38389#issuecomment-1777406762) with default of snappy compression, with lz4, and uncompressed (snappy clearly being the slowest, but the actual difference depends on which encoding is being used)

@mrocklin It seems like those files are not available using S3 anonymous access, am I right? Can you perhaps make one of those files available?

We have a public version here: s3://coiled-data/tpch/scale-1000/ That should be available with anonymous access

Okay, I took a quick look.
One thing that takes some time is dictionary decoding for the dictionary-encoded columns.
You can keep those columns dictionary-encoded in Arrow by passing read_dictionary=["l_returnflag", "l_linestatus", "l_shipinstruct", "l_shipmode"]. This seems to save around 25% CPU time (and also makes the data much more compact in memory).
Once that is done, I've got the following breakdown, roughly:

54% is Snappy decompression
16% is RLE decoding of dictionary indices
13% is PLAIN decoding of BYTE_ARRAY values
10% is PLAIN decoding of FLOAT and DOUBLE values

Something weird is that most columns out of this file have a single chunk, even though the file has 21 row groups. This doesn't look right:
>>> [(name, a.num_chunks) for name, a in zip(tab.column_names, tab.columns)]
[('l_orderkey', 1),
 ('l_partkey', 1),
 ('l_suppkey', 1),
 ('l_linenumber', 1),
 ('l_quantity', 1),
 ('l_extendedprice', 1),
 ('l_discount', 1),
 ('l_tax', 1),
 ('l_returnflag', 21),
 ('l_linestatus', 21),
 ('l_shipdate', 1),
 ('l_commitdate', 1),
 ('l_receiptdate', 1),
 ('l_shipinstruct', 21),
 ('l_shipmode', 21),
 ('l_comment', 1)]

>>> pf = pq.ParquetFile('~/arrow/data/lineitem/lineitem_0002072d-7283-43ae-b645-b26640318053.parquet')
>>> pf.metadata
<pyarrow._parquet.FileMetaData object at 0x7f236076dcb0>
  created_by: DuckDB
  num_columns: 16
  num_rows: 2568534
  num_row_groups: 21
  format_version: 1.0
  serialized_size: 29792

I mean that something is weird in the way the Parquet reader behaves, btw. The files don't seem to be at fault (except perhaps for using Snappy :-)).

Something weird is that most columns out of this file have a single chunk, even though the file has 21 row groups. This doesn't look right:

That's because of the use of pq.ParquetFile.read(..) (assuming you were using that here; @mrocklin's gist is using that, but your gist from earlier was using pq.read_table, which should result in much more chunks)
This ParquetFile.read() functions binds to parquet::arrow::FileReader::ReadTable, and I have noticed before that for some reason this concatenates the chunks somewhere in the read path.
On the other hand, ParquetFile.iter_batches() binds to FileReader::GetRecordBatchReader and the Dataset API / pq.read_table to FileReader::GetRecordBatchGenerator, and those two APIs will returns smaller batches (first per row group, but they also have a batch_size and will typically also return multiple chunks per row group).
# this file has 21 row groups
>>> file_path = "lineitem_0002072d-7283-43ae-b645-b26640318053.parquet"
>>> f = pq.ParquetFile(file_path)

# reading with ParquetFile.read gives a single chunk of data
>>> f.read()["l_orderkey"].num_chunks
1
# even when using the read_row_groups API
>>> f.read_row_groups([0, 1])["l_orderkey"].num_chunks
1
# only when using iter_batches, it's of course multiple chunks. The default batch_size here is 2**16,
# which even results in more batches than the number of row groups
>>> pa.Table.from_batches(f.iter_batches())["l_orderkey"].num_chunks
40
# we can make the batch_size larger
>>> pa.Table.from_batches(f.iter_batches(batch_size=128000))["l_orderkey"].num_chunks
21
# strangely it still seems to concatenate *across* row groups when further increasing the batch size
>>> pa.Table.from_batches(f.iter_batches(batch_size=2**17))["l_orderkey"].num_chunks
20

# pq.read_table uses the datasets API, but doesn't allow passing a batch size
>>> pq.read_table(file_path)["l_orderkey"].num_chunks
21
# in the datasets API, now the default batch size is 2**17 instead of 2**16 ...
>>> import pyarrow.dataset as ds
>>> ds.dataset(file_path, format="parquet").to_table()["l_orderkey"].num_chunks
21
# we can lower it (now each individual row group gets split, no combination of data of multiple row groups,
# I think because the GetRecordBatchGenerator uses a sub-generator per row group instead of a single iterator
# for the whole file as GetRecordBatchReader does)
>>> ds.dataset(file_path, format="parquet").to_table(batch_size=2**16)["l_orderkey"].num_chunks
42

So in summary, this is also a bit of a mess on our side (there are many different ways to read a parquet file ..). I had been planning to bring up that you might want to not use ParquetFile().read() in dask, because it's a bit slower because of returning a single chunk. Although if it's for the use case to convert to pandas later on, it might also not matter that much (although when using pyarrow strings, then it can matter).
On the Arrow side, we should maybe consider to make the default batch size a bit more uniform, and see if we want to use an actual batch size for the ReadTable code path as well.

I'm quite sure I was using pq.read_table above.

I ran perf against a debug build locally (Linux) and on an m6i VM and nothing significantly different seemed to appear. However, it naively seemed like the destructor of Status being the largest overhead was unexpected.
Thought it may have been related to https://issues.apache.org/jira/browse/ARROW-2400, and so reverted that w/ no significant change in the perf report. So just wanted to clarify if this is also expected?

@milesgranger You should run perf against a release build (with optimizations) otherwise the results will probably not be exploitable. A good practice is to select the RelWithDebInfo build type with CMake.

You can keep those columns dictionary-encoded in Arrow by passing read_dictionary=["l_returnflag", "l_linestatus", "l_shipinstruct", "l_shipmode"]. This seems to save around 25% CPU time (and also makes the data much more compact in memory).

I'm guessing that converting these to pandas dataframes would result in them being categorical dtype series.  Is that correct?

The files don't seem to be at fault (except perhaps for using Snappy :-)).

What would folks recommend as default compression?  LZ4?
If so, @milesgranger maybe it's easy to change the data generation scripts in some way with this change?  I'd be fine changing things in the benchmark if we think it's a good global recommendation.  (For context, I don't like changing things in benchmarks to make performance better because it results in over-tuning and non-realistic results, but if the change is good general practice as recommended by other people then it feels better I think).

I did these measurements on one CPU core. The overall read speed is around 220 MB/s from the local filesystem

@pitrou if it's easy for you, I'm curious how well this parallelizes.  If you do it on local four cores do you get ~800MB/s?  (assuming that your filesystem can go that fast, which seems likely)

And what machine did you get that number? (because above (https://github.com/apache/arrow/issues/38389#issuecomment-1777406762) you reported much higher single-threaded numbers)

I did these measurements on one CPU core. The overall read speed is around 220 MB/s from the local filesystem

@pitrou if it's easy for you, I'm curious how well this parallelizes. If you do it on local four cores do you get ~800MB/s? (assuming that your filesystem can go that fast, which seems likely)

Sorry for the delay. Basically, yes:
>>> %timeit pq.read_table('~/arrow/data/lineitem/lineitem_0002072d-7283-43ae-b645-b26640318053.parquet', use_threads=False, read_dictionary=['l_returnflag
...: ', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'])
575 ms ± 5.49 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

>>> pool = ThreadPoolExecutor(4)
>>> %timeit list(pool.map(lambda _: pq.read_table('~/arrow/data/lineitem/lineitem_0002072d-7283-43ae-b645-b26640318053.parquet', use_threads=False, read_d
...: ictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode']), range(4)))
596 ms ± 19.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Also, do note that the speed I'm reporting here (225 MB/s on one core) is relative to file size. Relative to in-memory Table size, this is more around 525 MB/s on one core.

Some low-level stats using perf stat -d -d -d:
 Performance counter stats for 'taskset -c 1 python -c import pyarrow.parquet as pq; [pq.read_table("~/arrow/data/lineitem/lineitem_0002072d-7283-43ae-b645-b26640318053.parquet", use_threads=False, read_dictionary=["l_returnflag", "l_linestatus", "l_shipinstruct", "l_shipmode"]).nbytes for i in range(10)]':

          6 051,90 msec task-clock                #    0,996 CPUs utilized
               806      context-switches          #  133,181 /sec
                 1      cpu-migrations            #    0,165 /sec
         1 340 104      page-faults               #  221,435 K/sec
    26 695 694 765      cycles                    #    4,411 GHz                      (20,17%)
     2 085 747 402      stalled-cycles-frontend   #    7,81% frontend cycles idle     (20,22%)
     9 165 304 095      stalled-cycles-backend    #   34,33% backend cycles idle      (20,25%)
    59 629 749 052      instructions              #    2,23  insn per cycle
                                                  #    0,15  stalled cycles per insn  (20,13%)
    10 417 615 407      branches                  #    1,721 G/sec                    (20,07%)
       102 060 991      branch-misses             #    0,98% of all branches          (20,27%)
    27 063 402 822      L1-dcache-loads           #    4,472 G/sec                    (20,25%)
       431 791 165      L1-dcache-load-misses     #    1,60% of all L1-dcache accesses  (20,12%)
   <not supported>      LLC-loads
   <not supported>      LLC-load-misses
     1 614 170 143      L1-icache-loads           #  266,721 M/sec                    (20,06%)
        30 033 666      L1-icache-load-misses     #    1,86% of all L1-icache accesses  (20,13%)
        24 183 320      dTLB-loads                #    3,996 M/sec                    (20,21%)
         7 451 324      dTLB-load-misses          #   30,81% of all dTLB cache accesses  (20,20%)
         1 845 633      iTLB-loads                #  304,968 K/sec                    (20,20%)
            97 899      iTLB-load-misses          #    5,30% of all iTLB cache accesses  (20,13%)
       200 049 623      L1-dcache-prefetches      #   33,056 M/sec                    (20,11%)
   <not supported>      L1-dcache-prefetch-misses

       6,074363585 seconds time elapsed

       4,552905000 seconds user
       1,494984000 seconds sys

At more than 2 instructions per cycle, it seems that we are not suffering much from cache or branch prediction misses.

Cool.  This is encouraging.  Thank you for following up on this.
On Wed, Dec 13, 2023 at 1:12 PM Antoine Pitrou @.***>
wrote:

Some low-level stats using perf stat -d -d -d:
Performance counter stats for 'taskset -c 1 python -c import pyarrow.parquet as pq; [pq.read_table("~/arrow/data/lineitem/lineitem_0002072d-7283-43ae-b645-b26640318053.parquet", use_threads=False, read_dictionary=["l_returnflag", "l_linestatus", "l_shipinstruct", "l_shipmode"]).nbytes for i in range(10)]':
      6 051,90 msec task-clock                #    0,996 CPUs utilized
           806      context-switches          #  133,181 /sec
             1      cpu-migrations            #    0,165 /sec
     1 340 104      page-faults               #  221,435 K/sec
26 695 694 765      cycles                    #    4,411 GHz                      (20,17%)
 2 085 747 402      stalled-cycles-frontend   #    7,81% frontend cycles idle     (20,22%)
 9 165 304 095      stalled-cycles-backend    #   34,33% backend cycles idle      (20,25%)
59 629 749 052      instructions              #    2,23  insn per cycle
                                              #    0,15  stalled cycles per insn  (20,13%)
10 417 615 407      branches                  #    1,721 G/sec                    (20,07%)
   102 060 991      branch-misses             #    0,98% of all branches          (20,27%)
27 063 402 822      L1-dcache-loads           #    4,472 G/sec                    (20,25%)
   431 791 165      L1-dcache-load-misses     #    1,60% of all L1-dcache accesses  (20,12%)

      LLC-loads
      LLC-load-misses
1 614 170 143      L1-icache-loads           #  266,721 M/sec                    (20,06%)
30 033 666      L1-icache-load-misses     #    1,86% of all L1-icache accesses  (20,13%)
24 183 320      dTLB-loads                #    3,996 M/sec                    (20,21%)
7 451 324      dTLB-load-misses          #   30,81% of all dTLB cache accesses  (20,20%)
1 845 633      iTLB-loads                #  304,968 K/sec                    (20,20%)
97 899      iTLB-load-misses          #    5,30% of all iTLB cache accesses  (20,13%)
200 049 623      L1-dcache-prefetches      #   33,056 M/sec                    (20,11%)
      L1-dcache-prefetch-misses
   6,074363585 seconds time elapsed

   4,552905000 seconds user
   1,494984000 seconds sys

At more than 2 instructions per cycle, it seems that we are not suffering
much from cache or branch prediction misses.
—
Reply to this email directly, view it on GitHub
https://github.com/apache/arrow/issues/38389#issuecomment-1854708228,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AACKZTCEMVBKGO7CCZML2JTYJIK5JAVCNFSM6AAAAAA6K5VD2SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJUG4YDQMRSHA
.
You are receiving this because you were mentioned.Message ID:
@.***>

I have also tried to regenerate the given file using different compressions and then compared reading performance:
>>> !ls -la lineitem-*
-rw-rw-r-- 1 antoine antoine 133922479 déc.  13 22:40 lineitem-lz4.pq
-rw-rw-r-- 1 antoine antoine 129419248 déc.  13 22:38 lineitem-snappy.pq
-rw-rw-r-- 1 antoine antoine 266474815 déc.  13 22:40 lineitem-uncompressed.pq
-rw-rw-r-- 1 antoine antoine  93395071 déc.  13 22:40 lineitem-zstd.pq

>>> %timeit pq.read_table('lineitem-snappy.pq', use_threads=False, read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'])
512 ms ± 12.5 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

>>> %timeit pq.read_table('lineitem-uncompressed.pq', use_threads=False, read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'])
252 ms ± 11 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

>>> %timeit pq.read_table('lineitem-lz4.pq', use_threads=False, read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'])
311 ms ± 13.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

>>> %timeit pq.read_table('lineitem-zstd.pq', use_threads=False, read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'])
456 ms ± 13.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

So we get:

uncompressed: 1 GB/s relative to file size, 1.2 GB/s relative to in-memory Table size
snappy: 252 MB/s relative to file size, 600 MB/s relative to in-memory Table size
lz4: 430 MB/s relative to file size, 990 MB/s relative to in-memory Table size
zstd: 205 MB/s relative to file size, 675 MB/s relative to in-memory Table size

Note that only the speeds relative to in-memory Table size are comparable, since the file sizes vary. Snappy is the slowest of all options, while producing a file size not much better than lz4 and strictly worse than zstd.

That said, ~1 GB/s for uncompressed PLAIN-encoded fixed-width data is still very mediocre. I think this has to with the fact that pq.read_table concatenates the row groups together instead of building one more chunk per row group:
>>> tab = pq.read_table('lineitem-uncompressed.pq', use_threads=False, read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'], columns=['l_or
...: derkey', 'l_partkey', 'l_suppkey', 'l_linenumber', 'l_shipdate', 'l_commitdate'])
>>> [{n: c.num_chunks} for n, c in zip(tab.column_names, tab.columns)]
[{'l_orderkey': 1},
 {'l_partkey': 1},
 {'l_suppkey': 1},
 {'l_linenumber': 1},
 {'l_shipdate': 1},
 {'l_commitdate': 1}]

If I deliberately read row groups separately for these PLAIN-encoded columns, I get almost twice the speed:
>>> %timeit pq.read_table('lineitem-uncompressed.pq', use_threads=False, read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'], columns=['l_
...: orderkey', 'l_partkey', 'l_suppkey', 'l_linenumber', 'l_shipdate', 'l_commitdate'])
67.1 ms ± 1.39 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

>>> f = pq.ParquetFile('lineitem-uncompressed.pq', read_dictionary=['l_returnflag', 'l_linestatus', 'l_shipinstruct', 'l_shipmode'])
>>> %timeit [f.read_row_group(i, use_threads=False, columns=['l_orderkey', 'l_partkey', 'l_suppkey', 'l_linenumber', 'l_shipdate', 'l_commitdate']) for i in range(f.nu
...: m_row_groups)]
36.6 ms ± 841 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

@pitrou the different compression benchmarks were running using hot file cache, right? I'd be surprised if uncompressed would be faster than lz4 without the linux file cache (even if you have the latest NVMe) as reading from disk should be slower than decompression.
Also LZ4 should be (roughly) 5GB/s, Snappy 2Gb/s and ZSTD should be 1GB/s on a i7-9700k (2018) on a single thread. I know it hurts data locality and copying around data will never help, but the results keep me thinking what's missing.

@pitrou the different compression benchmarks were running using hot file cache, right?

Yes, running on hot file cache. Benchmarking with cold cache is tedious and makes numbers much more difficult to compare (IO speeds vary probably much more than single-thread CPU speeds).

Also LZ4 should be (roughly) 5GB/s, Snappy 2Gb/s and ZSTD should be 1GB/s on a i7-9700k (2018) on a single thread.

Agreed, though do note that it's on a high-clocked CPU (server CPUs tend to have much lower clock speeds). And in practice it may depend on the corpus as well.


make the package compatible with go error package

Fix: https://github.com/go-errors/errors/issues/32

it's out already as 1.3.0

@meetme2meat I ended up reverting this because it broke other people's workflows.
Unfortunately that means that the only path forward to fixing this is either a v2, or a separate API that does what you want.

yes, a v2 would be fine I can separate PR around it.

I am currently use code like this (I saved the if code to a template):
// g and h are functions in an external library not in my code.
func g() error {
	...
}

func h() error {
	...
}

func f() (err error) {
	errRaw := g()
	if errRaw != nil {
		err = errors.Wrap(errRaw, 0)
		return
	}

	errRaw = h()
	if errRaw != nil {
		err = errors.Wrap(errRaw, 0)
		return
	}
	return
}

This forwards only non nil errors. If error is nil I have no use for wrapping it. I have to check for nil anyway.
You could also use err directly for errRaw but for me it is more clear like this.

I have to check for nil anyway.

Yes, check like if err != nil could be avoided just by using return errors.Wrap(err, 0)

I think it makes sense to include the behavior in this PR in a version 2. In some cases you do have to check for nil anyway but in other cases it would simplify code if one could write:
func f() error {
    // …

    return errors.Wrap(g(), 0) // Always return != nil using latest go-errors
}

Especially now that Wrap and WrapPrefix includes the guards below it looks like the above code would work even though it won't:
func WrapPrefix(e interface{}, prefix string, skip int) error {
	if e == nil {
		return nil
	}

	// …
}

One thing that becomes slightly harder when New/Wrap/WrapPrefix returns error is to access the stack trace. I suggest that a new function errors.ErrorStack(err) is added to handle this.
Then you could write errors.ErrorStack(err) where you today write errors.Wrap(err, 0).ErrorStack(). The nil handling of this function can be discussed. It could be made to only work with non nil input:
// ErrorStack is a convenience function that returns an error's error message
// and callstack of a non nil error. If the provided error has no callstack
// one will be created to the call-site of this function.
func ErrorStack(err error) string {
	return Wrap(err, 1).(*Error).ErrorStack()
}

Or it could be made to handle nil by returning an empty string or <nil> or perhaps <nil> with a stack trace:
// ErrorStack is a convenience function that returns an error's error message
// and callstack. If the provided error has no callstack, one will be created to
// the call-site of this function. Calling this function with nil is most likely a bug
// but it will return a callstack to this function with the error message `<nil>`.
func ErrorStack(err error) string {
	if err != nil {
		return New(nil).(*Error).ErrorStack()
	}
	return Wrap(err, 1).(*Error).ErrorStack()
}

Because of the go gotcha where nil is not nil it may be safest to use the second implementation. One may use the go-errors library by the book but other code may still return a typed nil somewhere which will make your code end up treating a success like and error which may propagate all the way to a central error handler where the stack trace is extracted.


Fix adding extension dataset when setting up a project

Closes #830

Thanks! Will be released in v0.19 (soon).