Details

    • Type: Sub-task Sub-task
    • Status: Closed Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.3.1
    • Component/s: JDO queries
    • Labels:
      None
    • Number of attachments :
      1

      Activity

      Hide
      Ralf Joachim added a comment -

      To resolve the synchronization at SQLStatementStore we not only need to care on the perpared statemnt but also on the counter and the resultset. While the best solution for the prepared statement again seams to be a ThreadLocal variable I don't think this would be the right solution for counter and resultset. I would pass the result set as parameter from executeQuery to processData method or merge both methods into one.

      The only reason to have a global counter is that we need to pass the inital index between the 3 bind methods. In my opinion we could evaluate the inital counters for each bind when building the SQL statement as these initial indizes will never change for an SQLStatementStore instance. With the global inital counters the counter itself can be moved into each of the bind methods. Does that make any sense to you?

      Show
      Ralf Joachim added a comment - To resolve the synchronization at SQLStatementStore we not only need to care on the perpared statemnt but also on the counter and the resultset. While the best solution for the prepared statement again seams to be a ThreadLocal variable I don't think this would be the right solution for counter and resultset. I would pass the result set as parameter from executeQuery to processData method or merge both methods into one. The only reason to have a global counter is that we need to pass the inital index between the 3 bind methods. In my opinion we could evaluate the inital counters for each bind when building the SQL statement as these initial indizes will never change for an SQLStatementStore instance. With the global inital counters the counter itself can be moved into each of the bind methods. Does that make any sense to you?
      Hide
      AHMAD HASSAN added a comment -

      I looked in to this matter.

      1) I agree that prepared statement should preferably by ThreadLocal.

      2) As only the executeQeury is the producer of resultSet and processData is the consumer of ResultSet, I agree with you that it doesn't make sense to keep it as global class attribute. But I am not in favor of merging executeQuery and processData together because I think structurally and functionally they are different methods. So I would prefer to return resultSet from executeQuery and pass it as a parameter to processData.

      3) As far as global counter is concerned. It is a bit complex issue. The global counter sets the offset for binding the parameters in the prepared Statement in three methods bindNewEntity, bindIdentity, bindOldEntity. Now the calculation of the these offset breakpoint for each of these methods is different. For example

      bindNewEntity - It involved SQLColumnInfo[] columns, SQLFieldInfo[] _fields, newentity to loop through the binding parameters
      bindIdentity - It involves SQLColumnInfo[] _ids
      bindOldEntity - It involved SQLColumnInfo[] columns, SQLFieldInfo[] _fields, oldentity to loop through the binding parameters

      I am not sure but I guess we have to run same kind of loop for all the three offsets required above inside build statement in order to calculate their offsets separately. Isn't it an overhead?

      I wouldn't mind to make the existing counter as ThreadLocal though because it will save us extra processing and also because the counter is being used by multiple methods.

      Regards, Ahmad

      Show
      AHMAD HASSAN added a comment - I looked in to this matter. 1) I agree that prepared statement should preferably by ThreadLocal. 2) As only the executeQeury is the producer of resultSet and processData is the consumer of ResultSet, I agree with you that it doesn't make sense to keep it as global class attribute. But I am not in favor of merging executeQuery and processData together because I think structurally and functionally they are different methods. So I would prefer to return resultSet from executeQuery and pass it as a parameter to processData. 3) As far as global counter is concerned. It is a bit complex issue. The global counter sets the offset for binding the parameters in the prepared Statement in three methods bindNewEntity, bindIdentity, bindOldEntity. Now the calculation of the these offset breakpoint for each of these methods is different. For example bindNewEntity - It involved SQLColumnInfo[] columns, SQLFieldInfo[] _fields, newentity to loop through the binding parameters bindIdentity - It involves SQLColumnInfo[] _ids bindOldEntity - It involved SQLColumnInfo[] columns, SQLFieldInfo[] _fields, oldentity to loop through the binding parameters I am not sure but I guess we have to run same kind of loop for all the three offsets required above inside build statement in order to calculate their offsets separately. Isn't it an overhead? I wouldn't mind to make the existing counter as ThreadLocal though because it will save us extra processing and also because the counter is being used by multiple methods. Regards, Ahmad
      Hide
      Ralf Joachim added a comment -

      1) Okay

      2) Passing resultset as parameter between the 2 methods is a good solution.

      3) To calculate the offsets at build statement we have to count the stored fields of new entity which is already done now. So there will be no additional overhead. Even if there would be a small overhaed at buildStatement I wouldn't care about that as it only happens once. On the other hand would the use of ThreadLocal also introduce some overhead but this one would happen at every execution of the query. I therefore prefer to calulate offset during buildStatement.

      offsetNewEntity = 1;
      offsetIdentity = offsetNewEntity + <number of stored fields of new entity>;
      offsetOldEntity = offsetIdentity + _ids.lenght;

      Show
      Ralf Joachim added a comment - 1) Okay 2) Passing resultset as parameter between the 2 methods is a good solution. 3) To calculate the offsets at build statement we have to count the stored fields of new entity which is already done now. So there will be no additional overhead. Even if there would be a small overhaed at buildStatement I wouldn't care about that as it only happens once. On the other hand would the use of ThreadLocal also introduce some overhead but this one would happen at every execution of the query. I therefore prefer to calulate offset during buildStatement. offsetNewEntity = 1; offsetIdentity = offsetNewEntity + <number of stored fields of new entity>; offsetOldEntity = offsetIdentity + _ids.lenght;
      Hide
      AHMAD HASSAN added a comment -

      I looked into the possibility of pre-calculating the offsets in the build statement. I came up with the following code for calculating the offsets. Just sharing it for the review.

              int _offsetNewEntity; //class attribute
              int _offsetIdentity; //class attribute
              int _offsetOldEntity; //class attribute
      
              //buildStatement Method
              int storedFields = 0;
              for (int i = 0; i < _fields.length; ++i) {
                  if (_fields[i].isStore()) {
                      SQLColumnInfo[] columns = _fields[i].getColumnInfo();
      
                      for (int j = 0; j < columns.length; j++) {
                          ++storedFields;
                      }
                  }
              }
              
              _offsetNewEntity = 1;
              _offsetIdentity = 1 + storedFields;
              _offsetOldEntity = _offsetIdentity + _ids.length;
      

      Regards, Ahmad

      Show
      AHMAD HASSAN added a comment - I looked into the possibility of pre-calculating the offsets in the build statement. I came up with the following code for calculating the offsets. Just sharing it for the review. int _offsetNewEntity; //class attribute int _offsetIdentity; //class attribute int _offsetOldEntity; //class attribute //buildStatement Method int storedFields = 0; for ( int i = 0; i < _fields.length; ++i) { if (_fields[i].isStore()) { SQLColumnInfo[] columns = _fields[i].getColumnInfo(); for ( int j = 0; j < columns.length; j++) { ++storedFields; } } } _offsetNewEntity = 1; _offsetIdentity = 1 + storedFields; _offsetOldEntity = _offsetIdentity + _ids.length; Regards, Ahmad
      Hide
      AHMAD HASSAN added a comment -

      P.S I agree with you that this will be much more efficient because this will only be executed once at the time of buildling the statement.

      Regards, Ahmad

      Show
      AHMAD HASSAN added a comment - P.S I agree with you that this will be much more efficient because this will only be executed once at the time of buildling the statement. Regards, Ahmad
      Hide
      AHMAD HASSAN added a comment -

      Patch Attached .

      One thing to review is the calculating of offsets. I ran the test cases and also try to see assigned values in debug mode. The tests run successfully.

      One other thing to note that, I am passing offset parameter explicitly in the bindIdentity method. Thid is so because this method is called for storeStatement and then for loadStatement. Both these Statement needs different offsets to starts with.

      Regards, Ahmad

      Show
      AHMAD HASSAN added a comment - Patch Attached . One thing to review is the calculating of offsets. I ran the test cases and also try to see assigned values in debug mode. The tests run successfully. One other thing to note that, I am passing offset parameter explicitly in the bindIdentity method. Thid is so because this method is called for storeStatement and then for loadStatement. Both these Statement needs different offsets to starts with. Regards, Ahmad
      Hide
      Ralf Joachim added a comment -

      Great work. Commited patch as is.

      Show
      Ralf Joachim added a comment - Great work. Commited patch as is.

        People

        • Assignee:
          AHMAD HASSAN
          Reporter:
          Ralf Joachim
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved: