Hi Rohit, Quick feedback based on the summary (will dive into the patch code later).
I don't think we should change the interface of the UpdateScript interface. This will break compile compatibility of any consuming application. And it also puts the responsibility to gather the right numbers on the user of the framework, not the framework itself. Rather the execute() methods on the statements should update an internal data structure that keeps track of the number of inserted/updated/deleted records. The ability to maintain such a data structure is provided in that the update callback is provided by the DataContext. The update callback can in other words have much richer functionality than what is exposed through it's interface. Hope it makes sense? Kasper 2013/8/26 Rohit <[email protected]>: > Hi Kasper, > > I've worked on code to provide UpdateStatus. > Just to be sure that I'm on right track I'm attaching a patch of > intermediate code changes. However they cover up following modules > completely: > couchdb, csv and core. > > Please let me know your thoughts on this implementation ? > If this is ok then I'll continue with the same approach to remaining > packages. > > Summary of changes: > > Edited UpdateableDataContext's executeUpdate to return UpdateStatus. Also > changed UpdateScript interface run method to return UpdateStatus because > executeUpdate calls run method internally. > Changed Row Insert/Update/Delete Builder "execute" method to return number > to affected rows. This info is then saved in UpdateStatus inside run method. > > Thanks, > Rohit > > > > On 21-Aug-2013, at 11:06 PM, Rohit <[email protected]> wrote: > > Hey Kasper, > > I'm out of t. Will be working on this over this weekend. > > Thanks, > Rohit. > > On 21-Aug-2013, at 6:17 PM, Kasper Sørensen <[email protected]> > wrote: > > Hi Rohit, > > It's been a little while now and I just wanted to check if you're > still on this task? IMO we gotta make some progress soon if we want to > include it in a release. Let it be heard if you're facing issues. > > Regards, > Kasper > > 2013/8/12 Kasper Sørensen <[email protected]>: > > Absolutely, that would be a wonderful start! :-) > > 2013/8/12 Rohit <[email protected]>: > > Agreed. > > In order to start contributing to the Metamodel. > As a first step can I provide you a patch of this, > then you can guide further ? > > Thanks, > Rohit > On 11-Aug-2013, at 3:30 PM, Kasper Sørensen <[email protected]> > wrote: > > The main development branch is 'master'. The .executeUpdate(...) method is > in the subinterface UpdateableDataContext. > > My point is that you can provide an update script with many operations in > one go. Take this example: > > UpdateStatus status = dataContext.executeUpdate(new UpdateScript() { > > public void run(UpdateCallback cb) { > cb.insertInto(table).value(...).execute(); > cb.insertInto(table).value(...).execute(); > cb.update(table).where(...).value(...).execute(); > cb.deleteFrom(table).where(...).execute(); > } > }); > > > > In this example we know that 2 records have been inserted. Some amount of > records have been updated (that may or may not be reported by the backing > datastore) and some amount of records have been deleted (that may or may > not have been reported by the backing datastore). > > Getting a list of affected records would be very complicated IMO, and would > only be consistent if done at the time of the transaction. That means that > if we even figure out how to do it, we also have to do it eagerly, which is > obviously not a good thing to do since I think getting that list is a > feature that would not apply to most use cases. > > The idea about doing this in a multithreaded/nonblocking fashion is > interesting. It would definately have to be an option in my opinion, since > I think the interface right now indicates that the executeUpdate method > will return succesfully when the update is done (ie. it throws those > exceptions that may arise during the update, and an unblocking variant > would not be able to do that). > > > 2013/8/11 Rohit <[email protected]> > > > My point is why do we need 3 methods to provide the status ? We can use > one method to provide the status (Returns a boolean if update operation a > done or not) and other to provide a list of affected rows/objects keys, > this would be less memory intensive. > And in case of deletion we can throw a custom exception stating " since > this is a delete operation we cannot provide affected rows". > > public interface UpdateStatus { > public <List or Collection> getAffectedRowsKeys(); or use > > getRowsKeys(); or getKeys() throws MetaModelException; > public Boolean isDone(); > > } > > > > I see following remote branches in git repo: > remotes/origin/HEAD -> origin/master > remotes/origin/hbase-module > remotes/origin/master > > Which is the current development branch as I couldn't find executeUpdate > in DataContext ? > > On 11-Aug-2013, at 1:04 PM, Kasper Sørensen < > [email protected]> wrote: > > I agree on the reasoning here, but getting a list of affected rows may > prove to be both quite difficult (from consistency point of view it > needs to be created immediately after some update - how can that be > implemented in an effecient manner (lazy, since you wouldn't always > want to get this list, but then consistency is gone), and how to > guarantee that those rows are even available (e.g. after a DELETE > statement in a JDBC database, you cannot get the rows anymore)), and > too memory intensive (updates can be anything from a single record to > millions of records). > > That's why I think the three methods (maybe with an additional fourth > method, returning the total num. of affected rows) is the best initial > set of methods. > > 2013/8/11 Rohit <[email protected]>: > > Having an abstraction for communicating status would be a cleaner > > approach. As it allows us to extend the functionality if required in future. > > For Example: > Lets say we are executing executeUpdate in multithreaded env. then we > > can provide methods to check the status of the update. (running or done). > > > Also rather than having methods returning int I think we should return > > a collection of Affected Rows. > > And rather than having multiple methods for insert/delete/update just > > have a single method returning the affected rows. > > > I think making so will make the API simpler yet functional. > > public interface UpdateStatus { > public <List or Collection> getAffectedRows(); or just use getRows() > } > > > > -RS > > On 10-Aug-2013, at 10:09 PM, Kasper Sørensen < > > [email protected]> wrote: > > > My experience with returning something like an > int/String/other-simple-type is that it seems to fit the purpose at > first, but then someone asks "couldn't we also get the status of > [something], too?" ... and then you regret not encapsulating the > return value so that you could just add another getter on it. > > 2013/8/10 Henry Saputra <[email protected]>: > > We could just make the executeUpdate just return number of elements > affected similar to relational databases. > > It would be simpler and serve similar purpose. > > - Henry > > > On Fri, Aug 9, 2013 at 12:15 AM, Kasper Sørensen < > [email protected]> wrote: > > Allow me to also bump this issue. > > Since this would be a change that would not be compile-time > incompatible with previous versions, I suggest we introduce at least > the return type of the method now. Since everyone moving the Apache > MetaModel from the old metamodel would anyways have to recompile, > > they > > would not feel the runtime incompatibility of void vs. > > SomeReturnType. > > > By now I suggest we just keep a very simple returned interface which > we can always choose to expand (implemented internally, not by > consumers). For instance something like: > > public interface UpdateStatus { > public int getInsertedRows(); > public int getUpdatedRows(); > public int getDeletedRows(); > } > > This should be quite easy to implement once and reuse for most of the > modules. For some datastores it might not be information that we can > retrieve, so the interface should also specify an extraordinary rule; > e.g. "if -1 is returned it means that it is not known how many rows > where inserted/updated/deleted" > > Kasper > > 2013/7/24 Kasper Sørensen <[email protected]>: > > In the current API design of MetaModel, the > > DataContext.executeUpdate(...) > > method is a void method. This was initially chosen because not all > implementations have the capability to report anything about a > > particular > > update. But some do, for instance the no. of inserted, updated or > > deleted > > records from a JDBC call. It would be nice to expose this > > information > > when > > available. > > My suggestion for this would be to let the > > DataContext.executeUpdate(...) > > method return an object with this information. All methods on the > > new > > object > > type would be optionally returning null, if no information is > > available. > > But > > when available, we can at least expose it this way. > > The change wouldn't have a major impact, since any project using > > MetaModel > > would already need to recompile because of the namespace change to > org.apache.metamodel. And the change would be compile-time > > compatible > > with > > having a void return type. > > > > > > > > >
