Skip to content

Conversation

GnaneshKunal
Copy link
Contributor

@GnaneshKunal GnaneshKunal commented Dec 28, 2022

FT.EXPLAIN/FT.EXPLAINCLI visualization

image

FT.PROFILE

image

GRAPH.EXPLAIN

image

GRAPH.PROFILE

image

@GnaneshKunal GnaneshKunal marked this pull request as ready for review January 5, 2023 05:48
egor-zalenski
egor-zalenski previously approved these changes Jan 10, 2023
- Use outline instead of border to apply the box edge styling outside the box.
- Add different profiling time for redisgraph PROFILE.
className={cx(styles.buttonIcon, styles.viewTypeIcon)}
onClick={onDropDownViewClick}
>
{isOpen && profileOptions.length > 1 && canCommandProfile && !summaryText && (
Copy link
Contributor

Choose a reason for hiding this comment

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

can be profileOptions length less than 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the check. profileOptions will always have 2 elements (PROFILE and EXPLAIN).

activeResultsMode={activeResultsMode}
resultsMode={resultsMode}
onQueryOpen={() => onQueryOpen(id)}
onQueryProfile={(profileType: ProfileQueryType) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please move this to the separate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using a prop function of the component here. Are you suggesting we can define the function inside the component?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, just a function inside the component
smth like that:

const handleQueryProfile = (profileType: ProfileQueryType) => {}
...
onQueryProfile={handleQueryProfile}

}
})

const canCommandProfile = isCommandAllowedForProfile(query.split(' ')[0].toLowerCase())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to move (' ')[0].toLowerCase() to the isCommandAllowedForProfile to not duplicate this everywhere in future
and it will be enough just use isCommandAllowedForProfile(query) in the condition below


const ALLOWED_PROFILE_COMMANDS = [...SEARCH_COMMANDS, ...GRAPH_COMMANDS]

export function isCommandAllowedForProfile(cmd: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we use arrow functions almost anywhere instead of function declaration , can you change it?
also, could you please add unit tests to each functions?

}

return null
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please add unit tests for generateGraphProfileQuery, generateSearchProfileQuery, generateProfileQueryForCommand

<div className={cx(styles.dropdownOption, styles.dropdownProfileOption)}>
<EuiIcon
className={styles.iconDropdownOption}
type={'visTagCloud'}
Copy link
Contributor

Choose a reason for hiding this comment

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

do not need {''} here just double quoutes

- Add unit tests for profile/explain commands generator
- Use arrow functions
export function isCommandAllowedForProfile(cmd: string) {
return ALLOWED_PROFILE_COMMANDS.includes(cmd)
export const isCommandAllowedForProfile = (query: string) => {
return ALLOWED_PROFILE_COMMANDS.includes(query.split(' ')[0].toLowerCase())
Copy link
Contributor

Choose a reason for hiding this comment

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

could you also add some check like
query?.split(' ')?.[0]?.toLowerCase() or query && ALLOWED_PROFILE_COMMANDS.includes(...) just to avoid some errors

]

describe('generateGraphProfileQuery', () => {

Copy link
Contributor

Choose a reason for hiding this comment

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

please, remove empty line

]

describe('generateSearchProfileQuery', () => {

Copy link
Contributor

Choose a reason for hiding this comment

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

the same


function generateGraphProfileQuery(query: string, type: ProfileQueryType) {
return [`graph.${type}`, ...query.split(' ').slice(1)].join(' ')
export function generateGraphProfileQuery(query: string, type: ProfileQueryType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you also change here to arrow function?

}

function generateSearchProfileQuery(query: string, type: ProfileQueryType) {
export function generateSearchProfileQuery(query: string, type: ProfileQueryType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the same


function generateSearchProfileQuery(query: string, type: ProfileQueryType) {
export function generateSearchProfileQuery(query: string, type: ProfileQueryType) {
const commandSplit = query.split(' ')
Copy link
Contributor

Choose a reason for hiding this comment

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

also, add checks to be sure that we cover null value of query

rsergeenko
rsergeenko previously approved these changes Jan 24, 2023
Copy link
Contributor

@rsergeenko rsergeenko left a comment

Choose a reason for hiding this comment

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

looks good, please resolve conflict

@rsergeenko rsergeenko changed the base branch from main to feature/RI-3726_profile_explain January 24, 2023 11:14
Copy link
Contributor

@rsergeenko rsergeenko left a comment

Choose a reason for hiding this comment

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

lgtm, changed target branch to feature/RI-3726_profile_explain
all bugfix please create from this branch (feature/RI-3726_profile_explain) with name starts fe/bugfix/RI-XXXX

we have pipelines to run tests from branches
fe/feature/** and fe/bugfix/** (will run FE tests)
feature/** (will run all tests after approve on circle ci)

@GnaneshKunal GnaneshKunal merged commit 6355f4c into feature/RI-3726_profile_explain Jan 25, 2023
GnaneshKunal added a commit that referenced this pull request Jan 31, 2023
GnaneshKunal added a commit that referenced this pull request Feb 6, 2023
@egor-zalenski egor-zalenski deleted the ri-explain-plugin branch March 17, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants