-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deep populate #1052
base: master
Are you sure you want to change the base?
Deep populate #1052
Conversation
criteria.where === null causing sails-memory to fail. retrieving foreign keys when fetching nextLayerParents, retieving only pks cause model populations to fail.
Adding a test.
…n each test. Correction of type verification bugs in deepCursor.
Construction of select of each path.
Impressive work @atiertant, @khalilTN, thanks! I think I speak for more than myself that this will take some time to digest. I would love to hear from @particlebanana, @devinivy, @tjwebb and @RWOverdijk. I do have a question which is something that @particlebanana was worried before: will this work in cross-adapter associations? Another one: how difficult/easy would it be to offload the cursor's work to an adapter for it to build the query in a more performant way? In my opinion the cursor you built should be a shim for adapters not supporting deep populate (currently none), as we want the adapters to this in a native manner when possible. Great work nonetheless! |
Holy smokes! Yes, this will take a little time to chew on. Any useful clues, @atiertant ? I like the in-code diagrams :) I think on the main deep populate issue, @dmarcelino, we did say that the shim should be completed before anything else, so this is great! |
@dmarcelino i think cross-adapter associations should work because,deepExec execute recusively exec like original populate but didn't test.for adapter deep support,the cursor wasn't wrote for this but we keep waterline logic : fist populate to make a query objet that could be execute by the adapter.so we could pass _criteria.paths to adapter who support deep. @devinivy if populate receive a dot notation it goes to populatePath.like populate add join in _criteria.joins,populatePath add joins classified hierarchically using a "path" (dot notation from root model) and useful informations to go in deeper path. |
Cool, thanks for clarifying! |
+1 |
@dmarcelino Any news about merging this PR ? |
@particlebanana thanks for the update. You know I'll want to take a crack at the beta ;) |
@particlebanana Excellent update. It would help if you could post the urls of the work in progress projects. Thanks! |
@particlebanana Thank you! I would love to have a look on the work in progress... |
@particlebanana Sounds good. Does this also come with a rough ETA? The reason I'm asking, is because we can then decide if it's worth waiting for, or if we should implement work-arounds. :) |
+1, really important |
@luish000 Also seems to fails with {type:json} Tried your suggested fix...
...and seems to work. Did you ever submit as PR? |
I've got mysql and redis updated to the latest driver spec- planning on hitting up postgresql and mongo this week to make a few tweaks from changes (the You can use the driver packs directly for transactions today if you want to take them for a spin. We're planning on getting them hooked up in the adapters as quickly as possible. With the mp CLI installed ( machinepack-mysql: ∑ mp compare ../waterline-driver-interface/
machinepack-mysql
- vs. -
waterline-driver-interface
No compatibility errors! \o/
No compatibility warnings! \o/
machinepack-mysql: ∑ If you're using Waterline in a Sails app, you'll also want to check out http://github.com/balderdashy/sails-hook-orm -- the new standalone version of the orm hook. Lots of cleanup in there, as well as the beginnings of the interface we hope to expose for database transactions via |
@mikermcneil what's the relation with deep populate ? maybe this not the right place to talk about it. |
I'm also not entirely sure. I guess there's a workaround in there (?) somewhere. |
@atiertant @RWOverdijk @dandanknight @s-devaney @benediktdertinger @devinivy @luislobo @komocode @kory-yhg @quantumlaser @sudoprimed @luish000 @gaurav21r @albertkim @tcury @fenos @wfpaisa @smith64fx Sorry if I missed anyone, this is a big thread. One of the more frustrating things about Waterline today is how often you need to dip into the realm of native queries to accomplish relatively simple database-specific tasks-- tasks that are achievable in other ORMs that specialize in only SQL databases or only Mongo. For instance, a join; or a Mongo geolocation query. Or projections. Or transactions. Or dynamic database connections. These things come up all the time in apps we work on too (remember that we're using Sails and Waterline in production products), and we've always known we would need to figure this stuff out. Well, I'm tired of waiting around. We are making big, immediate changes that cut to the root of problem and empower adapter authors to implement better database-specific support for native features. Neither @particlebanana nor I have any interest in making Waterline SQL-specific or Mongo-specific. But we are going to prioritize the features we need for high-scale production apps. As @particlebanana mentioned, these lower-level steps (i.e. the driver interface, the drivers, and getting them working from Waterline adapters) are our focus, and we'd welcome help there. But continued requests for deep populate will be met with much grumpier Mikes and Codys. I will post pictures of my face covered in red lipstick and play with my laptop in the sprinkler. Cody will take up organized crime. To put it another way, we can't spend any cycles working on deep populate until the work we've been discussing is complete. Even then, we will only work on database-optimized deep populate-- that is, deep populates across associations within the same datastore and adapter. More on that in a sec. Re: xD/A populates (i.e. cross-datastore / cross-adapter populates)First, a state of the union: As you know, Waterline supports cross-adapter populates one level deep-- with one very specific caveat: You should not use // This is fine unless `User` and its `videos` are in different databases.
User.find().populate('videos', { limit: 15, sort: 'rating desc' }).exec(...); Waterline's behavior in this specific case ( As far as xD/A deep populates (deep populate across multiple datastores/adapters)Here are some of the interesting twists and turns:
I've personally put at least ~200 hours into solving this problem. And while I find it fascinating, there just hasn't been a lot of community interest-- despite me bringing up the effort quite frequently and soliciting help for about two years. Which is no big deal-- I completely get it. It's a hard problem to just jump into. Plus it's incredibly time consuming, not particularly rewarding in the immediate term, and it has to be done on nights and days off, with no pay. Normally, I'm the kind of lunatic who gets off on stuff like that. But unfortunately, I can't justify spending any more time on it. I only have a certain number of hours I can donate to open-source, and there are more important things I need to be focused on right now (see the links above, or the explanation of intra-adapter deep populates below, for example).
Deep populates within a single datastore/adapterDeep populate across models in the same association is also not natively supported by any other ORM/ODM; even Mongoose. That said, we are planning to explore support for this in Waterline-- but only for core adapters, and on a per-adapter basis (if it's not clear why, please read the section above). We will only implement deep populate after we are finished with the lower-level features we need to make this possible. Please don't take the above as a criticism. I just need to set expectations, and help clarify what's being asked. If you are interested in helping expedite this, we could absolutely use your feedback on the interface and help QA-ing the raw drivers. If you are a PLM/PM with a pool of hours available, or you work at a company with a 20% time rule, please consider using it to help us get this finished. To those of you who haven't contributed in the past: major props for reading this enormous GitHub comment 👍 That, in and of itself, is something admirable. And thanks for your vote of confidence. To everyone who has contributed to Waterline and/or Sails in the past or present: thank you so much. I really appreciate your support-- especially those of you who have been here since pretty much the beginning. You make my days better. |
@mikermcneil Thanks for the whole follow up. I was aware of most of what you explained but this is a great place to get all the pieces together. I'll put some hours on testing/implementing, specially MongoDB stuff. Thank you for all your work and time. |
BTW, I really think that even the simplest Populate should never go cross database/adapter. It's too much to as to an ORM. Having read your Waterline 2 PDF in the past, and having had the experience this last 20 something years developing, it's not something that is easy to implement. And actually, the real use case scenario is very unfrequent in my experience. @mikermcneil One thing that I would really love to have is caching. If you want, and whenever you have time, let's talk about how I've seen it implemented in other frameworks. I could work in implementing that, if we set up the proper methods/interface. |
@mikermcneil you know i use this deepPopulate on offshore in production ! this is working without any problems ! of course this could be optimised in adapter. i 'll work on it after xD/A deep populates. i'm sure that the code we wrote can do it with a bit of work. those who wants deepPopulate this is here @luislobo look a this offshore-PR #7 i used this code on an application who repeat many query in recursive code, the perf was just amazing... |
That'd be really helpful, thanks @luislobo (would you send me a PM on twitter w/ a time that works for you for a skype?) |
Sent! |
@atiertant I know for a fact this is being done in production right now. But, depending on the database(s) you're using, it does require tuning.. and that's my point. I don't want it to require tuning, I want it to work seamlessly. And when it comes time to use custom database-specific features/configuration (e.g. sharding/replicas/geolocation queries/native transactions), I want that to be easy too. @particlebanana and I are committed to making that happen. I'm sorry to cause offense; I don't mean to downplay the effort in this PR, and I'm certainly not suggesting it's not functioning for your production use case. I really appreciate the time you put into this, and this discussion has already had a big impact on the way we'll think about approaching deep populate in the future. If you'd like to take a look at WL2 and spec out some ideas re: xD/A populates, that's really awesome, and I'd love to see what you come up with. |
@mikermcneil hey mike you are not offensing me ! this deepPopulate doesn't need any database tuning, it's working seamlessly.let me explain you the concept :
this code will which database doesn't suport this ? |
I can't help but agree. I find myself looping and fetching now, because this functionality doesn't exist. Maybe we can have some sort of Q&A? As is evident by this PR, the community is willing to help :) The moment this became open source, it became our (the community) project. We all want to improve and extend waterline. So, let's do that and start an actual conversation on what can be done to get this in, or if you guys are planning something, perhaps open up and share those ideas. Sitting idle and hoping stuff will get added is not a nice way to work on a project. Something something 2 cents. |
@RWOverdijk i think the best way to help those who need this functionality is to write a sails-hook for offshore like sequelize one: sails-hook-sequelize.if someone could help... |
@mikermcneil and others: xD/A populates was alreay working look at tests but now where deep criteria too since 0.0.6 |
nice man! |
this is awesome, saves me so much code... |
does someone tryed sails-hook-orm-offshore ? |
I've not had a chance yet. But looking to put it into our test environment in the next couple of weeks. I'll let you know how it goes. |
@atiertant The issue with this approach is specifically when dealing with bidirectional "hasMany" associations that stretch across different databases, and combining that behavior with There is a way to work around this involving paging through intermediate records. It is not easy, but as I mentioned above, over the course of the last few years I've written tens of thousands of lines of code and tests that start down that path. I am open to any contributions there, and I am excited about the prospect of solving deeply-nested xD/A populates in Waterline someday. Again, I appreciate everyone's feedback and help-- especially @atiertant. Cross-database populates of any kind have never been properly implemented in any ORM that I know of. It is really cool that we were able to accomplish this for the first time (one level deep) in Waterline and Sails, while still implementing efficient queries when those joins are between models in the same database. Waterline will always continue to support xD/A populates as they are today (one level deep). But we are not actively working on deep populate or subqueries across different databases. Instead, we are interested in adding built-in support for efficient, scalable deep-populate between models in the same database. We feel that this is more pertinent for production use cases, including our own. I'm going to restate the summary of this conversation, just to be clear:
Hope that helps clear things up. Thanks again everyone for the feedback!
|
Update on deep populateJust wanted to give everyone an update on this: We're very close to releasing Waterline 0.13 now, with the updated adapters, which means that deep populate is starting to edge back up on the horizon again. Before looking into this, I'm planning on working with @particlebanana on improving schema migrations (i.e. manual and auto-migrations) and merging the concept of drivers with the concept of adapters in the spec. Note: In the mean time, for those of you who need to do a deep populate and end up rolling it yourself, you might take a look at the new
|
had dot notation in popolate like this :
this implement feature request https://github.com/balderdashy/waterline/issues/308