Skip to content
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

DM-34136: Initial version of Visit and CcdVisit for dp0.2. #47

Merged
merged 2 commits into from Apr 12, 2022

Conversation

ctslater
Copy link
Member

No description provided.

@ctslater ctslater force-pushed the tickets/DM-34136 branch 2 times, most recently from fce5de9 to c5f7a14 Compare March 22, 2022 21:54
columns:
- name: visit
'@id': '#Visit.visit'
datatype: long

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL doesn't have the numeric data type long. For signd 64-bit numbers you need to add the following attribute:

mysql:datatype: BIGINT

For 32-bit numbers:

mysql:datatype: INT

For unsigned types:

mysql:datatype: BIGINT UNSIGNED

or:

mysql:datatype: INT UNSIGNED

- name: physical_filter
'@id': '#Visit.physical_filter'
datatype: char
length: 32

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that MySQL-specific type definitions were added for each column of the schema, such as:

mysql:datatype: CHAR(32)

And, perhaps, in case strings are not exactly 32-character long a better option would be:

mysql:datatype: VARCHAR(32)

This would save quite a bit of space for very large tables.

description: ''
- name: ra
'@id': '#Visit.ra'
datatype: double

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mysql:datatype: DOUBLE

@iagaponenko
Copy link

Please, provide the following attribute for all columns:

mysql:datatype: 

@ctslater
Copy link
Member Author

ctslater commented Apr 8, 2022

I have pushed a new commit, but the fixes it contains are unrelated from the review comments.

I think that all of the changes that Igor is asking for are already accounted for in the mysql DDL that Felis generates. You can check this by running felis create-all --dry-run --engine-url 'mysql://localhost/blah' dp02_dc2.yaml, but for convenience I'll just paste the output here:

CREATE TABLE `dp02_test_PREOPS863_00`.`Visit` (
        visit BIGINT COMMENT '', 
        physical_filter CHAR(32) COMMENT '', 
        band CHAR(1) COMMENT '', 
        ra DOUBLE COMMENT '', 
        decl DOUBLE COMMENT '', 
        `skyRotation` DOUBLE COMMENT '', 
        azimuth DOUBLE COMMENT '', 
        altitude DOUBLE COMMENT '', 
        `zenithDistance` DOUBLE COMMENT '', 
        airmass DOUBLE COMMENT '', 
        `obsStart` TIMESTAMP NULL COMMENT '', 
        `expTime` DOUBLE COMMENT ''
)COMMENT='';

CREATE TABLE `dp02_test_PREOPS863_00`.`CcdVisit` (
        `visitId` BIGINT COMMENT '', 
        physical_filter CHAR(32) COMMENT '', 
        band CHAR(1) COMMENT '', 
        ra DOUBLE COMMENT '', 
        decl DOUBLE COMMENT '', 
        `zenithDistance` FLOAT COMMENT '', 
        `zeroPoint` FLOAT COMMENT '', 
        `psfSigma` FLOAT COMMENT '', 
        `skyBg` FLOAT COMMENT '', 
        `skyNoise` FLOAT COMMENT '', 
        detector BIGINT COMMENT '', 
        seeing DOUBLE COMMENT '', 
        `skyRotation` DOUBLE COMMENT '', 
        `expMidpt` TIMESTAMP NULL COMMENT '', 
        `expTime` DOUBLE COMMENT '', 
        `obsStart` TIMESTAMP NULL COMMENT '', 
        `darkTime` DOUBLE COMMENT '', 
        `xSize` BIGINT COMMENT '', 
        `ySize` BIGINT COMMENT '', 
        llcra DOUBLE COMMENT '', 
        llcdec DOUBLE COMMENT '', 
        ulcra DOUBLE COMMENT '', 
        ulcdec DOUBLE COMMENT '', 
        urcra DOUBLE COMMENT '', 
        urcdec DOUBLE COMMENT '', 
        lrcra DOUBLE COMMENT '', 
        lrcdec DOUBLE COMMENT ''
)COMMENT='';

The only remaining issue I see is that Felis doesn't seem to be using the mysql:datatype override on the DATETIME columns, but that's a felis bug and will have to be fixed separately. I don't think that's a blocker on merging this though, since the tables are already ingested?

Copy link

@iagaponenko iagaponenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided Felise generates the correct MySQL schema from this definition, this PR looks good to me.

@ctslater ctslater merged commit 70c5a6f into main Apr 12, 2022
@ctslater ctslater deleted the tickets/DM-34136 branch April 12, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants