- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2k
 
Oracle: add OracleChatMemory support #3601
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
Conversation
Signed-off-by: fanxt0218 <[email protected]>
| 
           @fanxt0218 Thanks for the PR! Could you add the schema and the corresponding integration test?  | 
    
          
 Ok, I've added oracle's schema and integration test classes and made it a new commit. Please review  | 
    
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
Signed-off-by: fanxt0218 <[email protected]>
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
| 
           @fanxt0218 Thanks for the heads up!  | 
    
| 
           Hi @fanxt0218 would you like to invite me as contributor to update this PR with some minor updates and specific stuff from Oracle (Im an Oracle employee)  | 
    
          
 Of course@psilberk! Please check the invitation message. In the   | 
    
| 
           Can you update the branch with the latest changes and resolve its conflicts? (I don't think I can do it).  | 
    
Signed-off-by: Fanxt_Ja <[email protected]>
Signed-off-by: fanxt0218 <[email protected]>
Signed-off-by: fanxt0218 <[email protected]>
          
 I have resolved the conflicts and merged the latest code.  | 
    
Signed-off-by: fanxt0218 <[email protected]>
Signed-off-by: psilberk <[email protected]>
Signed-off-by: Fanxt_Ja <[email protected]>
Signed-off-by: fanxt0218 <[email protected]>
| */ | ||
| 
               | 
          ||
| @SpringBootTest | ||
| @TestPropertySource(properties = { "spring.datasource.url=jdbc:oracle:thin:@localhost:1521/FREEPDB1", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use test containers for the test. Will fix when merging.
| </dependency> | ||
| 
               | 
          ||
| <!-- Oracle JDBC Driver --> | ||
| <dependency> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we prefer to use the test containers this dependency isn't required. Will remove when merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to post it for the test container dependency not the JDBC driver one. Will keep this in.
| </dependency> | ||
| 
               | 
          ||
| <!-- Testcontainers for Oracle --> | ||
| <dependency> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran into issues when dropping table using this test container. Switched to use the latest testcontainers-oracle-free which works fine. Will update when merging.
| @@ -0,0 +1,21 @@ | |||
| DROP TABLE SPRING_AI_CHAT_MEMORY; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use DROP TABLE IF EXISTS SPRING_AI_CHAT_MEMORY
| 
           Cherry-picked the commits, addressed the review comments, rebased and merged as 30af4e8. Thank you @fanxt0218 and @psilberk for your collaboration towards this PR!  | 
    
| 
           Hi @ilayaperumalg did we change the right .adoc?  | 
    
add the support for oracle